From e1439d72c3c7af9ef66b49f129e0bbd7aadd7cb6 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 19:26:07 -0700 Subject: [PATCH 1/7] ADFA-5589: stop killing the warm Gradle daemon on every activity recreate initialize() decided whether it could reuse the open connector by comparing whole InitializeProjectParams objects with ==. That class declares no equals and is not a data class, so == was reference equality -- against an object freshly deserialized from JSON-RPC on every call. The answer was always false, and the branch it guards, "Reusing connector instance...", had never run. Always-false means forceConnect on every initialize, which calls connector.disconnect(), and GradleConnector.disconnect() sends the running daemon StopWhenIdle. The client re-initializes whenever the editor activity is recreated -- every configuration change outside EditorActivityKt's configChanges, so a Dark/Light/Amoled switch, a locale change, a display-size change -- and each one therefore stopped the warm daemon and made the next build pay a full cold start. On a phone that is the most expensive single item in a build. The check now compares what actually decides whether a connector can serve a request: the project directory and the Gradle distribution. Those are the only two things a connector is bound to. Everything else in the params is per-call. Making InitializeProjectParams a data class does not fix this, which is why it is not the change here: buildId is generated fresh for every request, so value equality on the whole object stays false every time. It is also the only one of the tooling API's fourteen message types that is not already a data class -- the others are, so nothing else in that package carries the same trap. That is the whole sibling sweep, and it is why the comparison lives in a named function rather than inline. describesSameConnection is separate so it can be asserted. The rest of canReuseConnector reads private state that only a real connect populates, and a test that stubs the connect never sets it -- the first version of this test asserted forceConnect end to end and failed for exactly that reason, not because the fix was wrong. Four cases. The first fails without the fix, with the two params for one project reported as different connections; it also asserts that == says false for them, which is the defect stated directly. The other three are the bound in the other direction: a different directory, a different distribution, and nothing initialized yet. Found while investigating why ADFA-5514's daemon plot lost its series after a theme change. The chart was the symptom; the daemon really was being stopped. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../tooling/impl/ToolingApiServerImpl.kt | 43 ++++++++++++- .../tooling/impl/ToolingApiServerImplTest.kt | 61 +++++++++++++++++++ 2 files changed, 102 insertions(+), 2 deletions(-) diff --git a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt index dd5e0b8c26..781cbe36a7 100644 --- a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt +++ b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt @@ -102,6 +102,46 @@ internal class ToolingApiServerImpl : IToolingApiServer { private val log = LoggerFactory.getLogger(ToolingApiServerImpl::class.java) } + /** + * Whether the connector already open can serve [params]. + * + * A connector is bound to a project directory and a Gradle distribution and to nothing else, so + * those are the only fields that can make one unusable. Everything else in + * [InitializeProjectParams] is per-call. + * + * This used to compare the whole object, which made it *always false*: + * [InitializeProjectParams] is a plain class with no `equals`, so `==` is reference equality, + * and `params` arrives freshly deserialized from JSON-RPC on every call. The branch it guards + * -- "Reusing connector instance..." -- had therefore never run. + * + * The cost was not a slow path. A false answer means `forceConnect`, which disconnects the old + * connector, and `GradleConnector.disconnect()` sends the daemon `StopWhenIdle`. So every + * re-initialize stopped the warm daemon, and the client re-initializes on every activity + * recreate outside `EditorActivityKt`'s `configChanges` -- a theme change, a locale change, a + * display-size change. Each one cost the next build a cold daemon start (ADFA-5589). + * + * Making [InitializeProjectParams] a `data class` does not fix it: `buildId` is generated fresh + * per call, so value equality on the whole object stays false every time. + */ + private fun canReuseConnector(params: InitializeProjectParams): Boolean = + connector != null && connection != null && describesSameConnection(lastInitParams, params) + + /** + * Whether [a] and [b] name the same connection: the same project directory, served by the same + * Gradle distribution. + * + * Separate from [canReuseConnector] so it can be asserted. The rest of that check reads private + * state which only a real connect populates, and a test that stubs the connect never sets it. + */ + @VisibleForTesting + internal fun describesSameConnection( + a: InitializeProjectParams?, + b: InitializeProjectParams, + ): Boolean = + a != null && + a.directory == b.directory && + a.gradleDistribution == b.gradleDistribution + @VisibleForTesting internal fun getOrConnectProject( projectDir: File, @@ -179,8 +219,7 @@ internal class ToolingApiServerImpl : IToolingApiServer { } val stopWatch = StopWatch("Connection to project") - val isReinitializing = - connector != null && connection != null && params == lastInitParams + val isReinitializing = canReuseConnector(params) if (isReinitializing) { log.info("Project is being reinitialized") diff --git a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt index d0b127de24..74900228b6 100644 --- a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt +++ b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt @@ -1,7 +1,9 @@ package com.itsaky.androidide.tooling.impl import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.tooling.api.IToolingApiClient import com.itsaky.androidide.tooling.api.messages.BuildId +import com.itsaky.androidide.tooling.api.messages.GradleDistributionParams import com.itsaky.androidide.tooling.api.messages.InitializeProjectParams import com.itsaky.androidide.tooling.api.messages.result.InitializeResult import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult @@ -59,6 +61,65 @@ class ToolingApiServerImplTest { return MockServer(server, connector, connection) } + @Test + fun `GIVEN the same project twice THEN the connector can be reused`() { + val server = ToolingApiServerImpl() + + // Two separately built objects describing the same connection, which is what the client + // sends: every initialize request arrives freshly deserialized from JSON-RPC. + val first = testInitParams() + val second = testInitParams() + + // The guard used to be `params == lastInitParams`. InitializeProjectParams declares no + // equals, so that was reference equality between two distinct objects and answered false + // every time -- which meant forceConnect on every re-initialize, which disconnects the open + // connector, and GradleConnector.disconnect() sends the running daemon StopWhenIdle. The + // client re-initializes on every activity recreate outside EditorActivityKt's + // configChanges, so a theme, locale or display-size change killed the warm daemon and the + // next build paid a cold start (ADFA-5589). + assertThat(first == second).isFalse() + assertThat(server.describesSameConnection(first, second)).isTrue() + } + + @Test + fun `GIVEN a different project directory THEN the connector cannot be reused`() { + val server = ToolingApiServerImpl() + + // A connector is bound to its project directory, so reusing one across projects would run + // the next build against the previous project's connection. + assertThat( + server.describesSameConnection( + testInitParams(directory = "/does/not/exist"), + testInitParams(directory = "/somewhere/else"), + ), + ).isFalse() + } + + @Test + fun `GIVEN a different Gradle distribution THEN the connector cannot be reused`() { + val server = ToolingApiServerImpl() + + // The other thing a connector is bound to. Reusing one here would silently build with the + // distribution the user had just changed away from. + val wrapper = testInitParams() + val installation = + InitializeProjectParams( + directory = wrapper.directory, + gradleDistribution = GradleDistributionParams.forVersion("8.14.3"), + needsGradleSync = false, + buildId = BuildId.Unknown, + ) + + assertThat(server.describesSameConnection(wrapper, installation)).isFalse() + } + + @Test + fun `GIVEN nothing initialized yet THEN the connector cannot be reused`() { + val server = ToolingApiServerImpl() + + assertThat(server.describesSameConnection(null, testInitParams())).isFalse() + } + @Test fun `GIVEN any initialization params WHEN project init fails THEN report as failure`() { mockkObject(RootModelBuilder) From 628816468d15fa581b4cfb6f42bbb76346e9e0c3 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 21:14:20 -0700 Subject: [PATCH 2/7] ADFA-5589: make connector reuse safe, not just narrower Review of #1813 found the reuse path could hand back a dead connection, and could reuse one bound to a Gradle distribution the user had changed. - A connect that throws no longer leaves the disconnected connector and connection in place. Before this, a failed reconnect (bad installation dir, unreachable distribution) left the dead pair behind, and the next initialize whose params matched reused it -- CONNECTION_CLOSED on every build until the server process restarted. The old always-false guard self-healed from this; the fix removed that accident. - The reuse check now compares the wrapper's distributionUrl as well. GradleDistributionParams.WRAPPER carries no version: the tooling API resolves gradle-wrapper.properties inside connect() and freezes it into the connection, so two wrapper params compare equal across an upgrade. Main.checkGradleWrapper() can rewrite that file earlier in the same initialize, so the call that installs a new wrapper was exactly the one that reused the connection bound to the old one. - Directories compare as paths, not strings. forProjectDirectory takes a File, so a trailing separator -- or /sdcard against /storage/emulated/0, both live on Android -- was one connector but two strings, and answered "different project": disconnect, StopWhenIdle, cold start. - connector, connection and lastInitParams are @Volatile. Each initialize runs on whichever commonPool worker supplyAsync hands it and nothing else here pairs the writes with the reads, so a stale null reintroduced the bug intermittently. Tests: the four existing ones only exercised the pure comparison, and none could fail without the fix -- reverting it deleted the symbol. Added two that drive initialize() and assert the forceConnect the call site passes, which is what actually reaches disconnect(); mutating `forceConnect = !isReinitializing` to `true` fails the reuse one. Added one that fails a reconnect and asserts no dead connection survives; removing the null-out fails it. Also dropped an assertion that pinned the *absence* of equals on InitializeProjectParams, a class in another module, and fixed a test whose variable was named `installation` while building a GRADLE_VERSION distribution -- installation dir is the only non-wrapper value the app sends, so that is now the case covered. describesSameConnection and friends moved to the companion: they read no instance state, and every test was constructing a server it never used. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../tooling/impl/ToolingApiServerImpl.kt | 139 ++++++++++---- .../tooling/impl/ToolingApiServerImplTest.kt | 181 +++++++++++++++--- 2 files changed, 256 insertions(+), 64 deletions(-) diff --git a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt index 781cbe36a7..129a54214c 100644 --- a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt +++ b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt @@ -63,6 +63,7 @@ import org.gradle.tooling.internal.consumer.DefaultGradleConnector import org.jetbrains.annotations.VisibleForTesting import org.slf4j.LoggerFactory import java.io.File +import java.util.Properties import java.util.concurrent.CompletableFuture import java.util.concurrent.TimeUnit import java.util.concurrent.locks.ReentrantLock @@ -75,10 +76,24 @@ import kotlin.concurrent.withLock */ internal class ToolingApiServerImpl : IToolingApiServer { private var client: IToolingApiClient? = null + + // Volatile because the reuse decision crosses threads: each initialize() runs on whichever + // commonPool worker CompletableFuture.supplyAsync hands it, and nothing else in this class + // establishes a happens-before edge between one call's writes and the next call's reads. A + // stale null read here reintroduces ADFA-5589 intermittently. + @Volatile private var connector: GradleConnector? = null + + @Volatile private var connection: ProjectConnection? = null + + @Volatile private var lastInitParams: InitializeProjectParams? = null + /** The `distributionUrl` the wrapper named when [connection] was opened; null if not a wrapper. */ + @Volatile + private var lastWrapperDistribution: String? = null + @Suppress("ktlint:standard:backing-property-naming") private var _buildCancellationToken: CancellationTokenSource? = null @@ -99,48 +114,87 @@ internal class ToolingApiServerImpl : IToolingApiServer { get() = connector != null || connection != null companion object { + private const val WRAPPER_PROPERTIES = "gradle/wrapper/gradle-wrapper.properties" + private val log = LoggerFactory.getLogger(ToolingApiServerImpl::class.java) - } - /** - * Whether the connector already open can serve [params]. - * - * A connector is bound to a project directory and a Gradle distribution and to nothing else, so - * those are the only fields that can make one unusable. Everything else in - * [InitializeProjectParams] is per-call. - * - * This used to compare the whole object, which made it *always false*: - * [InitializeProjectParams] is a plain class with no `equals`, so `==` is reference equality, - * and `params` arrives freshly deserialized from JSON-RPC on every call. The branch it guards - * -- "Reusing connector instance..." -- had therefore never run. - * - * The cost was not a slow path. A false answer means `forceConnect`, which disconnects the old - * connector, and `GradleConnector.disconnect()` sends the daemon `StopWhenIdle`. So every - * re-initialize stopped the warm daemon, and the client re-initializes on every activity - * recreate outside `EditorActivityKt`'s `configChanges` -- a theme change, a locale change, a - * display-size change. Each one cost the next build a cold daemon start (ADFA-5589). - * - * Making [InitializeProjectParams] a `data class` does not fix it: `buildId` is generated fresh - * per call, so value equality on the whole object stays false every time. - */ - private fun canReuseConnector(params: InitializeProjectParams): Boolean = - connector != null && connection != null && describesSameConnection(lastInitParams, params) + /** + * Whether [a] and [b] name the same connection: the same project directory, served by the + * same Gradle distribution. A connector is bound to those two and nothing else; every other + * field of [InitializeProjectParams] is per-call. + * + * `a == b` cannot stand in for this. [InitializeProjectParams] declares no `equals`, so that + * is reference equality between objects that arrive freshly deserialized on every call, and + * it answered false every time (ADFA-5589). + */ + @VisibleForTesting + internal fun describesSameConnection( + a: InitializeProjectParams?, + b: InitializeProjectParams, + ): Boolean = + a != null && + sameDirectory(a.directory, b.directory) && + a.gradleDistribution == b.gradleDistribution + + /** + * Whether [a] and [b] name the same directory. + * + * As paths, not as strings: `GradleConnector.forProjectDirectory` takes a [File], so a + * trailing separator -- or `/sdcard` against `/storage/emulated/0`, both live on Android -- + * is one connector but two strings. Guessing wrong here only costs a needless reconnect. + */ + private fun sameDirectory( + a: String, + b: String, + ): Boolean { + val first = File(a) + val second = File(b) + return first == second || + runCatching { first.canonicalFile == second.canonicalFile }.getOrDefault(false) + } + + /** + * Whether a wrapper connection is still bound to the distribution the wrapper names. + * + * [GradleDistributionParams.WRAPPER] carries no version, so two wrapper params compare equal + * across a wrapper upgrade. The distribution is resolved from `gradle-wrapper.properties` + * inside `connect()` and frozen into the connection, and [Main.checkGradleWrapper] can + * rewrite that file earlier in this same initialize -- so without this the initialize that + * installs a new wrapper is exactly the one that reuses the connection bound to the old one. + */ + @VisibleForTesting + internal fun wrapperStillMatches( + params: InitializeProjectParams, + recorded: String?, + current: String? = wrapperDistributionUrl(params.directory), + ): Boolean = + params.gradleDistribution.type != GradleDistributionType.GRADLE_WRAPPER || + recorded == current + + /** The `distributionUrl` named by [directory]'s Gradle wrapper, or null if unreadable. */ + @VisibleForTesting + internal fun wrapperDistributionUrl(directory: String): String? = + runCatching { + File(directory, WRAPPER_PROPERTIES) + .takeIf(File::isFile) + ?.inputStream() + ?.use { stream -> Properties().apply { load(stream) } } + ?.getProperty("distributionUrl") + }.getOrNull() + } /** - * Whether [a] and [b] name the same connection: the same project directory, served by the same - * Gradle distribution. + * Whether the connector already open can serve [params] without reconnecting. * - * Separate from [canReuseConnector] so it can be asserted. The rest of that check reads private - * state which only a real connect populates, and a test that stubs the connect never sets it. + * Not merely a slow path when false: reconnecting disconnects the open connector, and + * `GradleConnector.disconnect()` sends the running daemon `StopWhenIdle` (ADFA-5589). */ @VisibleForTesting - internal fun describesSameConnection( - a: InitializeProjectParams?, - b: InitializeProjectParams, - ): Boolean = - a != null && - a.directory == b.directory && - a.gradleDistribution == b.gradleDistribution + internal fun canReuseConnector(params: InitializeProjectParams): Boolean = + connector != null && + connection != null && + describesSameConnection(lastInitParams, params) && + wrapperStillMatches(params, lastWrapperDistribution) @VisibleForTesting internal fun getOrConnectProject( @@ -160,6 +214,14 @@ internal class ToolingApiServerImpl : IToolingApiServer { connector?.disconnect() } + // Dropped before the connect, not after it. A connect that throws -- a bad installation + // directory, an unreachable distribution -- would otherwise leave the disconnected pair in + // place, and the next initialize whose params match would reuse a dead connection and fail + // every build with CONNECTION_CLOSED until the server process restarts. + this.connector = null + this.connection = null + this.lastWrapperDistribution = null + val connector = GradleConnector.newConnector().forProjectDirectory(projectDir) setupConnectorForGradleInstallation(connector, gradleDist) @@ -167,6 +229,12 @@ internal class ToolingApiServerImpl : IToolingApiServer { this.connector = connector this.connection = connection + this.lastWrapperDistribution = + if (gradleDist.type == GradleDistributionType.GRADLE_WRAPPER) { + wrapperDistributionUrl(projectDir.path) + } else { + null + } connector to connection } @@ -474,6 +542,7 @@ internal class ToolingApiServerImpl : IToolingApiServer { this.client = null this.buildCancellationToken = null this.lastInitParams = null + this.lastWrapperDistribution = null // wait for connections to close connectionCloseFuture.get() diff --git a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt index 74900228b6..b6e05d4a1b 100644 --- a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt +++ b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt @@ -1,7 +1,6 @@ package com.itsaky.androidide.tooling.impl import com.google.common.truth.Truth.assertThat -import com.itsaky.androidide.tooling.api.IToolingApiClient import com.itsaky.androidide.tooling.api.messages.BuildId import com.itsaky.androidide.tooling.api.messages.GradleDistributionParams import com.itsaky.androidide.tooling.api.messages.InitializeProjectParams @@ -13,14 +12,18 @@ import com.itsaky.androidide.tooling.impl.sync.RootModelBuilder import io.mockk.every import io.mockk.mockk import io.mockk.mockkObject +import io.mockk.mockkStatic import io.mockk.spyk +import io.mockk.unmockkAll import io.mockk.verify import org.gradle.tooling.GradleConnector import org.gradle.tooling.ProjectConnection +import org.junit.After import org.junit.Test import org.junit.runner.RunWith import org.junit.runners.JUnit4 import java.io.File +import java.nio.file.Files import java.util.concurrent.TimeUnit /** @@ -31,12 +34,19 @@ class ToolingApiServerImplTest { private fun testInitParams( directory: String = "/does/not/exist", forceSync: Boolean = false, + gradleDistribution: GradleDistributionParams = GradleDistributionParams.WRAPPER, ) = InitializeProjectParams( directory = directory, + gradleDistribution = gradleDistribution, needsGradleSync = forceSync, buildId = BuildId.Unknown, ) + @After + fun tearDown() { + unmockkAll() + } + private data class MockServer( val server: ToolingApiServerImpl, val connector: GradleConnector, @@ -52,7 +62,7 @@ class ToolingApiServerImplTest { every { server.getOrConnectProject( projectDir = any(), - forceConnect = true, + forceConnect = any(), initParams = any(), gradleDist = any(), ) @@ -63,32 +73,34 @@ class ToolingApiServerImplTest { @Test fun `GIVEN the same project twice THEN the connector can be reused`() { - val server = ToolingApiServerImpl() - // Two separately built objects describing the same connection, which is what the client - // sends: every initialize request arrives freshly deserialized from JSON-RPC. + // sends: every initialize request arrives freshly deserialized from JSON-RPC. The guard used + // to be `params == lastInitParams`, which is reference equality on this type and so answered + // false for exactly this case (ADFA-5589). val first = testInitParams() val second = testInitParams() - // The guard used to be `params == lastInitParams`. InitializeProjectParams declares no - // equals, so that was reference equality between two distinct objects and answered false - // every time -- which meant forceConnect on every re-initialize, which disconnects the open - // connector, and GradleConnector.disconnect() sends the running daemon StopWhenIdle. The - // client re-initializes on every activity recreate outside EditorActivityKt's - // configChanges, so a theme, locale or display-size change killed the warm daemon and the - // next build paid a cold start (ADFA-5589). - assertThat(first == second).isFalse() - assertThat(server.describesSameConnection(first, second)).isTrue() + assertThat(first).isNotSameInstanceAs(second) + assertThat(ToolingApiServerImpl.describesSameConnection(first, second)).isTrue() } @Test - fun `GIVEN a different project directory THEN the connector cannot be reused`() { - val server = ToolingApiServerImpl() + fun `GIVEN the same directory spelled differently THEN the connector can be reused`() { + // forProjectDirectory takes a File, so a trailing separator is the same connector. + assertThat( + ToolingApiServerImpl.describesSameConnection( + testInitParams(directory = "/does/not/exist"), + testInitParams(directory = "/does/not/exist/"), + ), + ).isTrue() + } + @Test + fun `GIVEN a different project directory THEN the connector cannot be reused`() { // A connector is bound to its project directory, so reusing one across projects would run // the next build against the previous project's connection. assertThat( - server.describesSameConnection( + ToolingApiServerImpl.describesSameConnection( testInitParams(directory = "/does/not/exist"), testInitParams(directory = "/somewhere/else"), ), @@ -97,27 +109,138 @@ class ToolingApiServerImplTest { @Test fun `GIVEN a different Gradle distribution THEN the connector cannot be reused`() { - val server = ToolingApiServerImpl() - - // The other thing a connector is bound to. Reusing one here would silently build with the - // distribution the user had just changed away from. + // The other thing a connector is bound to. forInstallationDir is what the app actually sends + // for the gradleInstallationDir preference, so that is the case worth pinning: reusing a + // connector here would silently build with the distribution the user just changed away from. val wrapper = testInitParams() val installation = - InitializeProjectParams( - directory = wrapper.directory, - gradleDistribution = GradleDistributionParams.forVersion("8.14.3"), - needsGradleSync = false, - buildId = BuildId.Unknown, - ) + testInitParams(gradleDistribution = GradleDistributionParams.forInstallationDir("/opt/gradle")) - assertThat(server.describesSameConnection(wrapper, installation)).isFalse() + assertThat(ToolingApiServerImpl.describesSameConnection(wrapper, installation)).isFalse() } @Test fun `GIVEN nothing initialized yet THEN the connector cannot be reused`() { + assertThat(ToolingApiServerImpl.describesSameConnection(null, testInitParams())).isFalse() + } + + @Test + fun `GIVEN the wrapper names a new distribution THEN the connector cannot be reused`() { + // Two WRAPPER params compare equal across a wrapper upgrade: the distribution is resolved + // from gradle-wrapper.properties inside connect() and frozen into the connection, so reusing + // it would keep building with the Gradle the user just upgraded away from. + assertThat( + ToolingApiServerImpl.wrapperStillMatches( + testInitParams(), + recorded = "https://example.invalid/gradle-8.14.3-bin.zip", + current = "https://example.invalid/gradle-9.0-bin.zip", + ), + ).isFalse() + } + + @Test + fun `GIVEN the wrapper is unchanged THEN the connector can be reused`() { + val url = "https://example.invalid/gradle-8.14.3-bin.zip" + assertThat( + ToolingApiServerImpl.wrapperStillMatches(testInitParams(), recorded = url, current = url), + ).isTrue() + } + + @Test + fun `GIVEN a non-wrapper distribution THEN the wrapper properties do not matter`() { + // An installation-dir connection does not read gradle-wrapper.properties at all. + assertThat( + ToolingApiServerImpl.wrapperStillMatches( + testInitParams(gradleDistribution = GradleDistributionParams.forInstallationDir("/opt/gradle")), + recorded = null, + current = "https://example.invalid/gradle-9.0-bin.zip", + ), + ).isTrue() + } + + @Test + fun `GIVEN a wrapper properties file THEN its distribution URL is read`() { + val project = Files.createTempDirectory("adfa5589").toFile() + val properties = File(project, "gradle/wrapper/gradle-wrapper.properties") + properties.parentFile.mkdirs() + properties.writeText("distributionUrl=https\\://example.invalid/gradle-8.14.3-bin.zip\n") + + assertThat(ToolingApiServerImpl.wrapperDistributionUrl(project.path)) + .isEqualTo("https://example.invalid/gradle-8.14.3-bin.zip") + assertThat(ToolingApiServerImpl.wrapperDistributionUrl("/does/not/exist")).isNull() + + project.deleteRecursively() + } + + @Test + fun `GIVEN the connector can be reused WHEN initializing THEN do not force a reconnect`() { + val (server) = mockkReusableServer(canReuse = true) + + server.initialize(testInitParams()).get(5, TimeUnit.SECONDS) + + // forceConnect is the whole point of the reuse check: it is what reaches + // connector.disconnect(), and that is what sends the running daemon StopWhenIdle. Asserting + // describesSameConnection alone would leave this wiring free to invert unnoticed. + verify(exactly = 1) { + server.getOrConnectProject( + projectDir = any(), + forceConnect = false, + initParams = any(), + gradleDist = any(), + ) + } + } + + @Test + fun `GIVEN the connector cannot be reused WHEN initializing THEN force a reconnect`() { + val (server) = mockkReusableServer(canReuse = false) + + server.initialize(testInitParams()).get(5, TimeUnit.SECONDS) + + verify(exactly = 1) { + server.getOrConnectProject( + projectDir = any(), + forceConnect = true, + initParams = any(), + gradleDist = any(), + ) + } + } + + @Test + fun `GIVEN a connect that fails THEN the dead connection is not left behind for reuse`() { val server = ToolingApiServerImpl() + val connector = mockk(relaxed = true) + every { connector.forProjectDirectory(any()) } returns connector + every { connector.connect() } returns + mockk(relaxed = true) andThenThrows RuntimeException("connect failed") + + mockkStatic(GradleConnector::class) + every { GradleConnector.newConnector() } returns connector + + server.getOrConnectProject(File("/does/not/exist"), forceConnect = true) + assertThat(server.isConnected).isTrue() + + // The reconnect disconnects the open connector before it opens the new one, so a connect + // that throws must not leave that dead pair in place: the next initialize whose params match + // would reuse it and fail every build with CONNECTION_CLOSED until the server restarts. + val reconnect = runCatching { server.getOrConnectProject(File("/does/not/exist"), forceConnect = true) } + + assertThat(reconnect.isFailure).isTrue() + assertThat(server.isConnected).isFalse() + } + + /** A server whose connect, project directory check and Gradle sync are all stubbed out. */ + private fun mockkReusableServer(canReuse: Boolean): MockServer { + mockkObject(RootModelBuilder) + every { + RootModelBuilder.build(any(), any()) + } returns ProjectSyncHelper.cacheFileForProject(File("/does/not/exist")) - assertThat(server.describesSameConnection(null, testInitParams())).isFalse() + val mocks = mockkToolingServer() + every { mocks.server.validateProjectDirectory(any()) } returns null + every { mocks.server.canReuseConnector(any()) } returns canReuse + return mocks } @Test From f1706f3d59a566fc108f44e2b7f7fee5a4d1ac32 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 9 Sep 2026 01:39:59 -0700 Subject: [PATCH 3/7] ADFA-5589: invalidate a dead connection, and read the pair once Review found the reuse check made two failure modes reachable that the old unconditional reconnect had been hiding. A build that fails with CONNECTION_CLOSED or CONNECTION_ERROR now drops the cached connector and connection. The previous commit guarded only the connect that throws; a connection broken any other way -- an external close, a killed daemon, a Gradle-side disconnect -- stayed cached and was handed to every later build, failing identically until the tooling server process restarted. Before the reuse check every initialize rebuilt the connector, so this healed by accident. The reuse fast path read connector and connection four times and dereferenced both with !!. That was safe while the fields were only nulled in shutdown(); the drop-before-connect added in the previous commit makes them null mid-flight on every reconnect, so a concurrent caller could pass the null checks and then throw on the !!. Each field is now read once into a local. Tests: 15, one new -- a connect, then a failure classified as CONNECTION_CLOSED, then isConnected is false. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../tooling/impl/ToolingApiServerImpl.kt | 33 +++++++++++++++++-- .../tooling/impl/ToolingApiServerImplTest.kt | 21 ++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt index 129a54214c..d0811c7506 100644 --- a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt +++ b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt @@ -206,8 +206,14 @@ internal class ToolingApiServerImpl : IToolingApiServer { ?: GradleDistributionParams.WRAPPER, ): Pair = withStopWatch("getOrConnectProject") { - if (!forceConnect && connector != null && connection != null) { - return@withStopWatch connector!! to connection!! + // Each field read once into a local. Four separate reads of two fields could pass the + // null checks and then throw on the !!: the reconnect below nulls both before it + // connects, so unlike before this fix they are null mid-flight on every reconnect, not + // only at shutdown. + val openConnector = connector + val openConnection = connection + if (!forceConnect && openConnector != null && openConnection != null) { + return@withStopWatch openConnector to openConnection } if (forceConnect) { @@ -551,7 +557,28 @@ internal class ToolingApiServerImpl : IToolingApiServer { null } - private fun getTaskFailureType(error: Throwable): Failure = + /** + * Classifies [error], and drops the cached connection when the error says it is dead. + * + * The reuse check is what makes this necessary. Before it, every initialize rebuilt the + * connector, so a connection broken by anything -- an external close, a killed daemon, a + * Gradle-side disconnect -- was silently replaced. Now a matching directory and distribution + * reuse it, so a pair that has gone bad would be handed to every later build and fail + * identically until the server process restarted. Guarding only the connect that throws + * covered one of the two ways this happens. + */ + @VisibleForTesting + internal fun getTaskFailureType(error: Throwable): Failure = + classifyTaskFailure(error).also { failure -> + if (failure == CONNECTION_CLOSED || failure == CONNECTION_ERROR) { + log.warn("Dropping the Gradle connection after {}; the next build will reconnect", failure) + connection = null + connector = null + lastWrapperDistribution = null + } + } + + private fun classifyTaskFailure(error: Throwable): Failure = when (error) { is BuildException -> BUILD_FAILED is BuildCancelledException -> BUILD_CANCELLED diff --git a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt index b6e05d4a1b..c296da743c 100644 --- a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt +++ b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt @@ -308,4 +308,25 @@ class ToolingApiServerImplTest { RootModelBuilder.build(initParams, any()) } } + + @Test + fun `GIVEN a build that reports a dead connection THEN the next initialize reconnects`() { + val server = ToolingApiServerImpl() + val connector = mockk(relaxed = true) + every { connector.forProjectDirectory(any()) } returns connector + every { connector.connect() } returns mockk(relaxed = true) + + mockkStatic(GradleConnector::class) + every { GradleConnector.newConnector() } returns connector + + server.getOrConnectProject(File("/does/not/exist"), forceConnect = true) + assertThat(server.isConnected).isTrue() + + // The reuse check made this reachable: before it, every initialize rebuilt the connector, so + // a connection broken by anything at all was silently replaced. Reusing a dead one fails + // every later build identically until the server process restarts. + assertThat(server.getTaskFailureType(IllegalStateException("connection closed"))) + .isEqualTo(TaskExecutionResult.Failure.CONNECTION_CLOSED) + assertThat(server.isConnected).isFalse() + } } From 539eea0d46cd8ba610e6fe0ab55a5089da7d731c Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 9 Sep 2026 06:43:37 -0700 Subject: [PATCH 4/7] ADFA-5589: mark a bad connection suspect instead of dropping it The previous commit nulled connector and connection when a build failed with CONNECTION_CLOSED or CONNECTION_ERROR. That looked equivalent to a reconnect and was worse than the bug it fixed, in three ways review found: - executeTasks dereferences `connection` with checkNotNull *before* its try, so the next build threw out of the CompletableFuture instead of reconnecting. isInitialized stays true, so nothing re-initialized on its own: every build failed identically until the user re-synced. The log line promising "the next build will reconnect" was false. - classifyTaskFailure maps any IllegalStateException to CONNECTION_CLOSED. That is fine for a result code and not fine for a destructive side effect: a sync or model-builder failure surfacing as an IllegalStateException tore down a perfectly healthy connection. - Nulling the fields skipped the disconnect, and the reconnect path's `connector?.disconnect()` then saw null -- so every occurrence stranded a Gradle connection, with its daemon client and threads, for the life of the server process. Now a connection failure sets connectionSuspect. canReuseConnector refuses to reuse while it is set, and executeTasks goes through connectionForBuild(), which reconnects via getOrConnectProject when it is -- so the replacement disconnects the old connector before opening a new one, and a build recovers on its own. A successful connect clears it. The broad classification is now safe because a false positive costs one extra reconnect rather than a working server. Tests: 16. The dead-connection test fails against the nulling form, and a new one pins that a BuildException leaves the connection alone. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../tooling/impl/ToolingApiServerImpl.kt | 69 ++++++++++++++----- .../tooling/impl/ToolingApiServerImplTest.kt | 27 +++++++- 2 files changed, 79 insertions(+), 17 deletions(-) diff --git a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt index d0811c7506..a08448b925 100644 --- a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt +++ b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt @@ -94,6 +94,17 @@ internal class ToolingApiServerImpl : IToolingApiServer { @Volatile private var lastWrapperDistribution: String? = null + /** + * Whether the open connection has failed a build in a way that suggests it is dead. + * + * Reconnecting is driven from here rather than by dropping the pair at the point of failure, + * so the replacement goes through [getOrConnectProject], which disconnects the old connector + * before it opens a new one. + */ + @Volatile + @VisibleForTesting + internal var connectionSuspect: Boolean = false + @Suppress("ktlint:standard:backing-property-naming") private var _buildCancellationToken: CancellationTokenSource? = null @@ -193,9 +204,31 @@ internal class ToolingApiServerImpl : IToolingApiServer { internal fun canReuseConnector(params: InitializeProjectParams): Boolean = connector != null && connection != null && + !connectionSuspect && describesSameConnection(lastInitParams, params) && wrapperStillMatches(params, lastWrapperDistribution) + /** + * The connection to build on, reconnecting first when the last build said it was dead. + * + * @throws IllegalStateException If nothing has been initialized, which is the caller's bug. + */ + private fun connectionForBuild(): ProjectConnection { + val params = lastInitParams + if (connectionSuspect && params != null) { + log.info("Reconnecting to Gradle: the previous build reported a dead connection") + return getOrConnectProject( + projectDir = File(params.directory), + forceConnect = true, + initParams = params, + ).second + } + + return checkNotNull(this.connection) { + "ProjectConnection has not been initialized. Cannot execute tasks." + } + } + @VisibleForTesting internal fun getOrConnectProject( projectDir: File, @@ -235,6 +268,7 @@ internal class ToolingApiServerImpl : IToolingApiServer { this.connector = connector this.connection = connection + this.connectionSuspect = false this.lastWrapperDistribution = if (gradleDist.type == GradleDistributionType.GRADLE_WRAPPER) { wrapperDistributionUrl(projectDir.path) @@ -394,10 +428,11 @@ internal class ToolingApiServerImpl : IToolingApiServer { Main.checkGradleWrapper() - val connection = - checkNotNull(this.connection) { - "ProjectConnection has not been initialized. Cannot execute tasks." - } + // Reconnects rather than asserting. A previous build that failed with CONNECTION_CLOSED + // leaves the pair suspect, and this dereference sits outside the try below -- so + // asserting here threw out of the future and every later build failed the same way + // until the user re-synced. + val connection = connectionForBuild() val builder = connection.newBuild() @@ -558,27 +593,29 @@ internal class ToolingApiServerImpl : IToolingApiServer { } /** - * Classifies [error], and drops the cached connection when the error says it is dead. + * Classifies [error], and marks the connection suspect when the error says it may be dead. * - * The reuse check is what makes this necessary. Before it, every initialize rebuilt the - * connector, so a connection broken by anything -- an external close, a killed daemon, a - * Gradle-side disconnect -- was silently replaced. Now a matching directory and distribution - * reuse it, so a pair that has gone bad would be handed to every later build and fail - * identically until the server process restarted. Guarding only the connect that throws - * covered one of the two ways this happens. + * A flag rather than dropping the pair here. Nulling the fields looked equivalent and was not: + * [executeTasks] dereferences `connection` with `checkNotNull` *before* its try, so the next + * build threw out of the future instead of reconnecting, and every build failed until the user + * re-synced. Nulling also skipped the disconnect, stranding the old connection's daemon client + * and threads for the life of the process. + * + * The classification is deliberately broad -- any [IllegalStateException] reads as + * CONNECTION_CLOSED -- so a false positive has to be cheap. Setting a flag costs one extra + * reconnect; tearing down a healthy connection cost a working server. */ @VisibleForTesting internal fun getTaskFailureType(error: Throwable): Failure = classifyTaskFailure(error).also { failure -> if (failure == CONNECTION_CLOSED || failure == CONNECTION_ERROR) { - log.warn("Dropping the Gradle connection after {}; the next build will reconnect", failure) - connection = null - connector = null - lastWrapperDistribution = null + log.warn("Marking the Gradle connection suspect after {}; the next build reconnects", failure) + connectionSuspect = true } } - private fun classifyTaskFailure(error: Throwable): Failure = + @VisibleForTesting + internal fun classifyTaskFailure(error: Throwable): Failure = when (error) { is BuildException -> BUILD_FAILED is BuildCancelledException -> BUILD_CANCELLED diff --git a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt index c296da743c..3ea4b95e62 100644 --- a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt +++ b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt @@ -16,6 +16,7 @@ import io.mockk.mockkStatic import io.mockk.spyk import io.mockk.unmockkAll import io.mockk.verify +import org.gradle.tooling.BuildException import org.gradle.tooling.GradleConnector import org.gradle.tooling.ProjectConnection import org.junit.After @@ -327,6 +328,30 @@ class ToolingApiServerImplTest { // every later build identically until the server process restarts. assertThat(server.getTaskFailureType(IllegalStateException("connection closed"))) .isEqualTo(TaskExecutionResult.Failure.CONNECTION_CLOSED) - assertThat(server.isConnected).isFalse() + + // Suspect, not dropped. Dropping it here nulled a field that executeTasks dereferences + // outside its try, so the next build threw out of the future rather than reconnecting -- + // and skipped the disconnect, stranding the old connection for the life of the process. + assertThat(server.isConnected).isTrue() + assertThat(server.connectionSuspect).isTrue() + } + + @Test + fun `GIVEN a classification that is not a connection failure THEN the connection is left alone`() { + val server = ToolingApiServerImpl() + val connector = mockk(relaxed = true) + every { connector.forProjectDirectory(any()) } returns connector + every { connector.connect() } returns mockk(relaxed = true) + + mockkStatic(GradleConnector::class) + every { GradleConnector.newConnector() } returns connector + + server.getOrConnectProject(File("/does/not/exist"), forceConnect = true, initParams = testInitParams()) + + // The classifier is deliberately broad, so anything it drives has to be cheap on a false + // positive. A build failure is not a reason to reconnect. + assertThat(server.getTaskFailureType(BuildException("failed", RuntimeException()))) + .isEqualTo(TaskExecutionResult.Failure.BUILD_FAILED) + assertThat(server.connectionSuspect).isFalse() } } From 03f2f22d9ac09fe7cc0f5504a84fd0f0872c1c6b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 9 Sep 2026 08:54:31 -0700 Subject: [PATCH 5/7] ADFA-5589: shut the Gradle daemon watcher down with the server GradleDaemonWatcher.shutdown() had no caller. Reported by hal-eisen-adfa on #1812, where the code is in the base commit rather than the diff, so it lands here instead -- this is the file that owns the watcher. Three consequences, all his: - the watcher's thread outlived server shutdown, and an in-flight poll chain went on scanning ProcessHandle.descendants() for up to a minute - onExit().thenRun { client?.onGradleDaemonExited(pid) } could fire into a client whose RPC channel was being torn down; shutdown() sets client to null, so that was a race rather than a guaranteed no-op - shutdown() was dead code, which made onBuildStarted's note about the scheduler rejecting work after shutdown describe an unreachable state Called before the client is cleared, so no later poll can report into a channel that is going away. Through the lazy delegate rather than the property: touching the property would construct a watcher, and its scheduler, only to shut it down again on a server that never ran a build. Tests: 28 in the module, one new -- shutdown stops the scheduler. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../tooling/impl/ToolingApiServerImpl.kt | 30 +++++++++++++++---- .../tooling/impl/GradleDaemonWatcherTest.kt | 12 ++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt index a08448b925..369afb752d 100644 --- a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt +++ b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt @@ -509,12 +509,15 @@ internal class ToolingApiServerImpl : IToolingApiServer { * Finds the Gradle daemon and reports it to the client, so the memory chart can plot the process * that actually holds the build's heap (ADFA-5514). */ - private val daemonWatcher by lazy { - GradleDaemonWatcher( - onStarted = { pid -> client?.onGradleDaemonStarted(pid) }, - onExited = { pid -> client?.onGradleDaemonExited(pid) }, - ) - } + private val lazyDaemonWatcher = + lazy { + GradleDaemonWatcher( + onStarted = { pid -> client?.onGradleDaemonStarted(pid) }, + onExited = { pid -> client?.onGradleDaemonExited(pid) }, + ) + } + + private val daemonWatcher by lazyDaemonWatcher private fun notifyBuildFailure(result: BuildResult) { client?.onBuildFailed(result) @@ -555,6 +558,21 @@ internal class ToolingApiServerImpl : IToolingApiServer { buildCancellationToken?.cancel() buildCancellationToken = null + // Before the client goes, so no further poll can report a daemon into an RPC channel + // that is being torn down. Through the delegate rather than the property: touching the + // property would build a watcher, and its scheduler, only to shut it down again on a + // server that never ran a build. + // + // This was never called at all, so the watcher's thread outlived server shutdown and an + // in-flight poll chain went on scanning descendants for up to a minute. It also made + // GradleDaemonWatcher.shutdown() dead code, and onBuildStarted's note about the + // scheduler rejecting work after shutdown describe a state nothing could reach. + if (lazyDaemonWatcher.isInitialized()) { + log.info("Stopping the Gradle daemon watcher...") + runCatching { daemonWatcher.shutdown() } + .onFailure { log.warn("Could not stop the Gradle daemon watcher", it) } + } + val connection = this.connection val connector = this.connector this.connection = null diff --git a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcherTest.kt b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcherTest.kt index 2b6ac12aa5..470d042112 100644 --- a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcherTest.kt +++ b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcherTest.kt @@ -213,4 +213,16 @@ class GradleDaemonWatcherTest { /** Every poll attempt, plus the initial schedule. */ const val MAX_SCHEDULES = GradleDaemonWatcher.MAX_POLL_ATTEMPTS + 1 } + + @Test + fun `shutdown stops the scheduler`() { + // It had no caller at all, so the watcher's thread outlived server shutdown and an in-flight + // poll chain went on scanning descendants for up to a minute -- and onBuildStarted's note + // about the scheduler rejecting work after shutdown described a state nothing could reach. + val scheduler = mockk(relaxed = true) + + watcher(scheduler = scheduler).shutdown() + + verify(exactly = 1) { scheduler.shutdownNow() } + } } From 7a830d15abe1bb4e3434fb2a4f50e0e940dbe01b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 9 Sep 2026 13:39:12 -0700 Subject: [PATCH 6/7] ADFA-5589: make runBuild total, and stop the classifier deciding recovery Three review rounds each found a defect in the previous round's fix, all in the same two lines. Patching the reported symptom kept moving the throw instead of covering it, so this changes the shape instead. runBuild takes an onFailure and catches Throwable around the whole action. Both callers used to guard only the part that talks to Gradle, so everything before it -- resolving the connection, checking the wrapper, preparing the build -- escaped as a raw exception and the client got an ExecutionException where it expected a classified failure. That was reported as a checkNotNull outside the try; the fix replaced it with a reconnect on the same line, still outside the try, and the next review reported it again. "Build already in progress" is left to throw: it is a caller error, not a build outcome. getTaskFailureType is pure again. Marking the connection suspect from a classifier meant initialize's catch condemned connections over sync failures -- classifyTaskFailure maps any IllegalStateException to CONNECTION_CLOSED, so a model builder throwing one was enough. And the penalty was never the "one extra reconnect" I claimed when I added it: reconnecting calls GradleConnector.disconnect(), which sends the running daemon StopWhenIdle. That is this ticket's own bug, on a new trigger, and the javap trace proving disconnect() sends StopWhenIdle was in this same branch's commit history at the time. The flag is now set at the one site that can tell a dead connection from a bad project: a build that ran against it and failed. Also from the same review: - getOrConnectProject disconnected through the field two lines after taking it into a local, under a comment about reading each field once. A concurrent reconnect could have it disconnect the new connector and drop the old one undisconnected. - shutdown() stopped the daemon watcher before DefaultGradleConnector .close() stopped the daemons, so the exit was rejected by a dead scheduler and the client never heard the daemon it was plotting had gone. The watcher now stops after the daemons, and the client is cleared after the wait rather than before it -- best effort even so, since the client's own channel is going away at the same time. Tests: 17. The three replacing the old pair drive the behaviour rather than the flag: classification leaves the connection alone, a suspect connection is replaced on the next build (and the old connector is disconnected, not stranded), and a reconnect that throws returns a classified failure. Removing runBuild's catch fails the last one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../tooling/impl/ToolingApiServerImpl.kt | 177 ++++++++++-------- .../tooling/impl/ToolingApiServerImplTest.kt | 63 +++++-- 2 files changed, 148 insertions(+), 92 deletions(-) diff --git a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt index 369afb752d..4cba52c98e 100644 --- a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt +++ b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt @@ -88,7 +88,8 @@ internal class ToolingApiServerImpl : IToolingApiServer { private var connection: ProjectConnection? = null @Volatile - private var lastInitParams: InitializeProjectParams? = null + @VisibleForTesting + internal var lastInitParams: InitializeProjectParams? = null /** The `distributionUrl` the wrapper named when [connection] was opened; null if not a wrapper. */ @Volatile @@ -213,7 +214,8 @@ internal class ToolingApiServerImpl : IToolingApiServer { * * @throws IllegalStateException If nothing has been initialized, which is the caller's bug. */ - private fun connectionForBuild(): ProjectConnection { + @VisibleForTesting + internal fun connectionForBuild(): ProjectConnection { val params = lastInitParams if (connectionSuspect && params != null) { log.info("Reconnecting to Gradle: the previous build reported a dead connection") @@ -250,7 +252,10 @@ internal class ToolingApiServerImpl : IToolingApiServer { } if (forceConnect) { - connector?.disconnect() + // The local, not the field. Re-reading it here is what the locals above exist to + // avoid: a concurrent reconnect can install a new connector between the two reads, + // and this would then disconnect that one and drop the one it meant to close. + openConnector?.disconnect() } // Dropped before the connect, not after it. A connect that throws -- a bad installation @@ -285,11 +290,9 @@ internal class ToolingApiServerImpl : IToolingApiServer { } override fun initialize(params: InitializeProjectParams): CompletableFuture { - return runBuild { - val start = System.currentTimeMillis() - try { - return@runBuild doInitialize(params, start) - } catch (err: Throwable) { + val start = System.currentTimeMillis() + return runBuild( + onFailure = { err -> log.error("Failed to initialize project", err) notifyBuildFailure( BuildResult( @@ -298,8 +301,12 @@ internal class ToolingApiServerImpl : IToolingApiServer { durationMs = System.currentTimeMillis() - start, ), ) - return@runBuild InitializeResult.Failure(getTaskFailureType(err)) - } + // No suspicion of the connection from here. A sync failure is about the project or + // its model, and condemning the connection over one costs a cold daemon start. + InitializeResult.Failure(getTaskFailureType(err)) + }, + ) { + doInitialize(params, start) } } @@ -407,8 +414,28 @@ internal class ToolingApiServerImpl : IToolingApiServer { override fun isServerInitialized(): CompletableFuture = CompletableFuture.supplyAsync { isInitialized } override fun executeTasks(message: TaskExecutionMessage): CompletableFuture { - return runBuild { - val start = System.currentTimeMillis() + val start = System.currentTimeMillis() + return runBuild( + onFailure = { error -> + log.error("Failed to run tasks: {}", message.tasks, error) + notifyBuildFailure( + result = + BuildResult( + tasks = message.tasks, + buildId = message.buildId, + durationMs = System.currentTimeMillis() - start, + ), + ) + val failure = getTaskFailureType(error) + // The one place a dead connection can be told apart from a bad project: a build + // that ran against it and failed. The next build reconnects rather than reusing it. + if (failure == CONNECTION_CLOSED || failure == CONNECTION_ERROR) { + log.warn("Marking the Gradle connection suspect after {}; the next build reconnects", failure) + connectionSuspect = true + } + TaskExecutionResult(false, failure) + }, + ) { if (!isServerInitialized().get()) { log.error("Cannot execute tasks: {}", PROJECT_NOT_INITIALIZED) return@runBuild TaskExecutionResult(false, PROJECT_NOT_INITIALIZED) @@ -428,10 +455,10 @@ internal class ToolingApiServerImpl : IToolingApiServer { Main.checkGradleWrapper() - // Reconnects rather than asserting. A previous build that failed with CONNECTION_CLOSED - // leaves the pair suspect, and this dereference sits outside the try below -- so - // asserting here threw out of the future and every later build failed the same way - // until the user re-synced. + // Reconnects rather than asserting, when the last build said the connection was dead. + // A reconnect can itself throw -- an unreachable distribution, a bad installation dir -- + // and runBuild's catch is what turns that into a classified failure instead of an + // exceptionally-completed future. val connection = connectionForBuild() val builder = connection.newBuild() @@ -451,30 +478,17 @@ internal class ToolingApiServerImpl : IToolingApiServer { this.buildCancellationToken = GradleConnector.newCancellationTokenSource() builder.withCancellationToken(this.buildCancellationToken!!.token()) - try { - builder.run() - this.buildCancellationToken = null - notifyBuildSuccess( - result = - BuildResult( - tasks = message.tasks, - buildId = message.buildId, - durationMs = System.currentTimeMillis() - start, - ), - ) - return@runBuild TaskExecutionResult.SUCCESS - } catch (error: Throwable) { - log.error("Failed to run tasks: {}", message.tasks, error) - notifyBuildFailure( - result = - BuildResult( - tasks = message.tasks, - buildId = message.buildId, - durationMs = System.currentTimeMillis() - start, - ), - ) - return@runBuild TaskExecutionResult(false, getTaskFailureType(error)) - } + builder.run() + this.buildCancellationToken = null + notifyBuildSuccess( + result = + BuildResult( + tasks = message.tasks, + buildId = message.buildId, + durationMs = System.currentTimeMillis() - start, + ), + ) + TaskExecutionResult.SUCCESS } } @@ -558,21 +572,6 @@ internal class ToolingApiServerImpl : IToolingApiServer { buildCancellationToken?.cancel() buildCancellationToken = null - // Before the client goes, so no further poll can report a daemon into an RPC channel - // that is being torn down. Through the delegate rather than the property: touching the - // property would build a watcher, and its scheduler, only to shut it down again on a - // server that never ran a build. - // - // This was never called at all, so the watcher's thread outlived server shutdown and an - // in-flight poll chain went on scanning descendants for up to a minute. It also made - // GradleDaemonWatcher.shutdown() dead code, and onBuildStarted's note about the - // scheduler rejecting work after shutdown describe a state nothing could reach. - if (lazyDaemonWatcher.isInitialized()) { - log.info("Stopping the Gradle daemon watcher...") - runCatching { daemonWatcher.shutdown() } - .onFailure { log.warn("Could not stop the Gradle daemon watcher", it) } - } - val connection = this.connection val connector = this.connector this.connection = null @@ -588,6 +587,18 @@ internal class ToolingApiServerImpl : IToolingApiServer { // Stop all daemons log.info("Stopping all Gradle Daemons...") DefaultGradleConnector.close() + + // After the daemons, not before. The exit is reported through + // handle.onExit().thenRun { scheduler.execute { ... } }, so a scheduler already + // shut down rejects it and the client never hears that the daemon it is + // plotting has gone. Through the delegate rather than the property: touching + // the property would build a watcher, and its scheduler, only to shut it down + // again on a server that never ran a build. + if (lazyDaemonWatcher.isInitialized()) { + log.info("Stopping the Gradle daemon watcher...") + runCatching { daemonWatcher.shutdown() } + .onFailure { log.warn("Could not stop the Gradle daemon watcher", it) } + } } // update the initialization flag before cancelling future @@ -598,7 +609,6 @@ internal class ToolingApiServerImpl : IToolingApiServer { log.info("Cancelling awaiting future...") Main.future?.cancel(true) - this.client = null this.buildCancellationToken = null this.lastInitParams = null this.lastWrapperDistribution = null @@ -606,34 +616,32 @@ internal class ToolingApiServerImpl : IToolingApiServer { // wait for connections to close connectionCloseFuture.get() + // After the wait, not before. Stopping the daemons is what makes the watcher report + // their exit, and that report goes through `client` -- cleared first, it was a silent + // no-op and the client's chart kept the daemon's last value. Best effort even so: the + // client's own channel is going away at the same time. + this.client = null + log.info("Shutdown request completed.") null } /** - * Classifies [error], and marks the connection suspect when the error says it may be dead. + * Classifies [error]. Pure: it reads nothing and changes nothing. * - * A flag rather than dropping the pair here. Nulling the fields looked equivalent and was not: - * [executeTasks] dereferences `connection` with `checkNotNull` *before* its try, so the next - * build threw out of the future instead of reconnecting, and every build failed until the user - * re-synced. Nulling also skipped the disconnect, stranding the old connection's daemon client - * and threads for the life of the process. + * It briefly marked the connection suspect, which was wrong twice over. The classification is + * deliberately broad -- any [IllegalStateException] reads as CONNECTION_CLOSED -- and + * `initialize`'s catch routes every sync failure through it, so a model builder throwing an + * IllegalStateException condemned a connection that had just been opened successfully. And the + * penalty was never the "one extra reconnect" its comment claimed: reconnecting calls + * `GradleConnector.disconnect()`, which sends the running daemon `StopWhenIdle` -- the cold + * start this whole ticket exists to prevent, on a new trigger. * - * The classification is deliberately broad -- any [IllegalStateException] reads as - * CONNECTION_CLOSED -- so a false positive has to be cheap. Setting a flag costs one extra - * reconnect; tearing down a healthy connection cost a working server. + * Recovery now lives at the one site that can tell a dead connection from a bad project: a + * build that failed against it. See [executeTasks]. */ @VisibleForTesting internal fun getTaskFailureType(error: Throwable): Failure = - classifyTaskFailure(error).also { failure -> - if (failure == CONNECTION_CLOSED || failure == CONNECTION_ERROR) { - log.warn("Marking the Gradle connection suspect after {}; the next build reconnects", failure) - connectionSuspect = true - } - } - - @VisibleForTesting - internal fun classifyTaskFailure(error: Throwable): Failure = when (error) { is BuildException -> BUILD_FAILED is BuildCancelledException -> BUILD_CANCELLED @@ -650,7 +658,24 @@ internal class ToolingApiServerImpl : IToolingApiServer { action() } - private inline fun runBuild(crossinline action: () -> T): CompletableFuture = + /** + * Runs [action] as the one build in progress, turning anything it throws into [onFailure]'s + * result rather than an exceptionally-completed future. + * + * The catch spans the whole action deliberately. Each caller used to guard only the part that + * talks to Gradle, so everything before it -- resolving a connection, checking the wrapper, + * preparing the build -- escaped as a raw exception, and the client got an ExecutionException + * where it expected a classified failure. That is one bug this file has now had twice, in two + * different statements on the same line, because the fix each time moved the throw instead of + * covering it. + * + * "Build already in progress" is left to throw: it is a caller error rather than a build + * outcome, and it is raised before this takes ownership of the flag. + */ + private inline fun runBuild( + crossinline onFailure: (Throwable) -> T, + crossinline action: () -> T, + ): CompletableFuture = supplyAsync { if (isBuildInProgress) { log.error("Cannot run build, build is already in progress!") @@ -661,6 +686,8 @@ internal class ToolingApiServerImpl : IToolingApiServer { daemonWatcher.onBuildStarted() try { action() + } catch (error: Throwable) { + onFailure(error) } finally { isBuildInProgress = false } diff --git a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt index 3ea4b95e62..7f26a10f58 100644 --- a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt +++ b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt @@ -4,6 +4,7 @@ import com.google.common.truth.Truth.assertThat import com.itsaky.androidide.tooling.api.messages.BuildId import com.itsaky.androidide.tooling.api.messages.GradleDistributionParams import com.itsaky.androidide.tooling.api.messages.InitializeProjectParams +import com.itsaky.androidide.tooling.api.messages.TaskExecutionMessage import com.itsaky.androidide.tooling.api.messages.result.InitializeResult import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import com.itsaky.androidide.tooling.api.messages.result.isSuccessful @@ -16,7 +17,6 @@ import io.mockk.mockkStatic import io.mockk.spyk import io.mockk.unmockkAll import io.mockk.verify -import org.gradle.tooling.BuildException import org.gradle.tooling.GradleConnector import org.gradle.tooling.ProjectConnection import org.junit.After @@ -25,6 +25,7 @@ import org.junit.runner.RunWith import org.junit.runners.JUnit4 import java.io.File import java.nio.file.Files +import java.util.concurrent.CompletableFuture import java.util.concurrent.TimeUnit /** @@ -311,7 +312,7 @@ class ToolingApiServerImplTest { } @Test - fun `GIVEN a build that reports a dead connection THEN the next initialize reconnects`() { + fun `GIVEN a connection failure THEN classifying it changes nothing`() { val server = ToolingApiServerImpl() val connector = mockk(relaxed = true) every { connector.forProjectDirectory(any()) } returns connector @@ -321,37 +322,65 @@ class ToolingApiServerImplTest { every { GradleConnector.newConnector() } returns connector server.getOrConnectProject(File("/does/not/exist"), forceConnect = true) - assertThat(server.isConnected).isTrue() - // The reuse check made this reachable: before it, every initialize rebuilt the connector, so - // a connection broken by anything at all was silently replaced. Reusing a dead one fails - // every later build identically until the server process restarts. + // Classification is pure. It briefly marked the connection suspect, and initialize's catch + // routes every sync failure through it -- so a model builder throwing an + // IllegalStateException condemned a connection opened moments earlier, and the next build's + // reconnect sent the warm daemon StopWhenIdle. ADFA-5589's own bug, on a new trigger. assertThat(server.getTaskFailureType(IllegalStateException("connection closed"))) .isEqualTo(TaskExecutionResult.Failure.CONNECTION_CLOSED) - - // Suspect, not dropped. Dropping it here nulled a field that executeTasks dereferences - // outside its try, so the next build threw out of the future rather than reconnecting -- - // and skipped the disconnect, stranding the old connection for the life of the process. + assertThat(server.connectionSuspect).isFalse() assertThat(server.isConnected).isTrue() - assertThat(server.connectionSuspect).isTrue() } @Test - fun `GIVEN a classification that is not a connection failure THEN the connection is left alone`() { + fun `GIVEN a build failed on a dead connection THEN the next build reconnects rather than reusing it`() { val server = ToolingApiServerImpl() val connector = mockk(relaxed = true) every { connector.forProjectDirectory(any()) } returns connector - every { connector.connect() } returns mockk(relaxed = true) + val first = mockk(relaxed = true) + val second = mockk(relaxed = true) + every { connector.connect() } returns first andThen second mockkStatic(GradleConnector::class) every { GradleConnector.newConnector() } returns connector server.getOrConnectProject(File("/does/not/exist"), forceConnect = true, initParams = testInitParams()) + // Set directly: only doInitialize writes it, and that needs a real project directory. + // connectionForBuild reconnects to the project the last initialize named. + server.lastInitParams = testInitParams() + assertThat(server.connectionForBuild()).isSameInstanceAs(first) + + // What the flag is for: the connection stays in place and the *next build* replaces it, + // which is what reaches connector.disconnect() and releases the old daemon client and its + // threads. Nulling the fields at the point of failure skipped that and stranded them. + server.connectionSuspect = true - // The classifier is deliberately broad, so anything it drives has to be cheap on a false - // positive. A build failure is not a reason to reconnect. - assertThat(server.getTaskFailureType(BuildException("failed", RuntimeException()))) - .isEqualTo(TaskExecutionResult.Failure.BUILD_FAILED) + assertThat(server.connectionForBuild()).isSameInstanceAs(second) assertThat(server.connectionSuspect).isFalse() + verify(atLeast = 1) { connector.disconnect() } + } + + @Test + fun `GIVEN a reconnect that throws THEN the build reports a failure rather than completing exceptionally`() { + val (server) = mockkToolingServer() + every { server.isServerInitialized() } returns CompletableFuture.completedFuture(true) + every { server.connectionForBuild() } throws IllegalStateException("cannot reconnect") + + mockkObject(Main) + every { Main.checkGradleWrapper() } returns Unit + + // Everything before the Gradle call used to sit outside the try, so a throw here escaped as + // an ExecutionException where the client expected a classified failure -- and the same line + // did it twice, first as a checkNotNull and then as the reconnect that replaced it. + val result = + server + .executeTasks(TaskExecutionMessage(tasks = listOf("assembleDebug"), buildId = BuildId.Unknown)) + .get(5, TimeUnit.SECONDS) + + assertThat(result.isSuccessful).isFalse() + assertThat(result.failure).isEqualTo(TaskExecutionResult.Failure.CONNECTION_CLOSED) + // And this is the one site that marks the connection suspect. + assertThat(server.connectionSuspect).isTrue() } } From 901f38b9b475b6528eb0a2b67039275ac7ee8aef Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 9 Sep 2026 15:30:56 -0700 Subject: [PATCH 7/7] ADFA-5589: condemn the connection only for a build that ran against it Widening runBuild's catch to the whole action fixed an escaped exception and created a new way to reach this ticket's own bug. classifyTaskFailure reads any IllegalStateException as CONNECTION_CLOSED, so a throw from Main.checkGradleWrapper, doPrepareBuild or configureFrom -- none of which says anything about the connection -- marked it suspect from that catch, and the next build's reconnect sent the running daemon StopWhenIdle. A cold start, from a setup failure. The mark moves onto builder.run() itself. runBuild's catch still turns everything into a classified result; only a build that actually ran against the connection now says the connection is dead. isBuildInProgress becomes an AtomicBoolean with compareAndSet, and isInitialized gains @Volatile. Both are read and written from whichever commonPool worker supplyAsync hands the call, which is the reason the five fields above them are already @Volatile -- these two were missed in that sweep. As a plain read-then-write, two concurrent executeTasks calls could both see false and proceed: two builds against one connection and one cancellation token. buildCancellationToken needs nothing; it already reads and writes under its own lock. The daemon-watcher shutdown leaves this branch. #1816 (ADFA-5659) carries it to stage on its own, with the caller-level tests it needs and without the ordering claim that did not hold; keeping a second, divergent copy here would be worse than waiting for that to land. Tests: 31 in the module, two new. A setup failure that leaves the connection alone fails if the mark goes back on runBuild's catch. The refusal test is named for what it pins: a second build is refused, not that the CAS is atomic -- an interleaving is not reproducible on demand, so the compareAndSet is argued from the threading model, not pinned. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../tooling/impl/ToolingApiServerImpl.kt | 91 ++++++++++--------- .../tooling/impl/ToolingApiServerImplTest.kt | 54 ++++++++++- 2 files changed, 102 insertions(+), 43 deletions(-) diff --git a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt index 4cba52c98e..9116e39908 100644 --- a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt +++ b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt @@ -66,6 +66,7 @@ import java.io.File import java.util.Properties import java.util.concurrent.CompletableFuture import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.locks.ReentrantLock import kotlin.concurrent.withLock @@ -115,11 +116,22 @@ internal class ToolingApiServerImpl : IToolingApiServer { set(value) = cancellationTokenAccessLock.withLock { _buildCancellationToken = value } /** Whether the project has been initialized or not. */ + @Volatile var isInitialized: Boolean = false private set - /** Whether a build or project synchronization is in progress. */ - private var isBuildInProgress: Boolean = false + /** + * Whether a build or project synchronization is in progress, as a CAS rather than a + * read-then-write. + * + * Same reason the fields above are @Volatile: every call runs on whichever commonPool worker + * CompletableFuture.supplyAsync hands it. As a plain Boolean, two concurrent executeTasks calls + * could both read false and both proceed -- two builds against one connection and one + * cancellation token -- and a worker could go on seeing a stale true and refuse every build for + * the life of the server. ([buildCancellationToken] needs nothing here; it already reads and + * writes under its own lock.) + */ + private val buildInProgress = AtomicBoolean(false) /** Whether the server has a live connection to Gradle. */ val isConnected: Boolean @@ -426,14 +438,7 @@ internal class ToolingApiServerImpl : IToolingApiServer { durationMs = System.currentTimeMillis() - start, ), ) - val failure = getTaskFailureType(error) - // The one place a dead connection can be told apart from a bad project: a build - // that ran against it and failed. The next build reconnects rather than reusing it. - if (failure == CONNECTION_CLOSED || failure == CONNECTION_ERROR) { - log.warn("Marking the Gradle connection suspect after {}; the next build reconnects", failure) - connectionSuspect = true - } - TaskExecutionResult(false, failure) + TaskExecutionResult(false, getTaskFailureType(error)) }, ) { if (!isServerInitialized().get()) { @@ -478,7 +483,20 @@ internal class ToolingApiServerImpl : IToolingApiServer { this.buildCancellationToken = GradleConnector.newCancellationTokenSource() builder.withCancellationToken(this.buildCancellationToken!!.token()) - builder.run() + try { + builder.run() + } catch (error: Throwable) { + // Marked here, not in runBuild's catch. That catch covers the whole action, which + // is what makes a setup failure into a classified result instead of an escaped + // exception -- but classifyTaskFailure reads *any* IllegalStateException as + // CONNECTION_CLOSED, so marking from there condemned the connection over a throw + // from checkGradleWrapper, doPrepareBuild or configureFrom. The next build then + // disconnected, which sends the running daemon StopWhenIdle: this ticket's own bug, + // reached from a setup failure. A build that actually ran against the connection is + // the only failure that says anything about it. + markSuspectIfConnectionFailure(error) + throw error + } this.buildCancellationToken = null notifyBuildSuccess( result = @@ -523,15 +541,12 @@ internal class ToolingApiServerImpl : IToolingApiServer { * Finds the Gradle daemon and reports it to the client, so the memory chart can plot the process * that actually holds the build's heap (ADFA-5514). */ - private val lazyDaemonWatcher = - lazy { - GradleDaemonWatcher( - onStarted = { pid -> client?.onGradleDaemonStarted(pid) }, - onExited = { pid -> client?.onGradleDaemonExited(pid) }, - ) - } - - private val daemonWatcher by lazyDaemonWatcher + private val daemonWatcher by lazy { + GradleDaemonWatcher( + onStarted = { pid -> client?.onGradleDaemonStarted(pid) }, + onExited = { pid -> client?.onGradleDaemonExited(pid) }, + ) + } private fun notifyBuildFailure(result: BuildResult) { client?.onBuildFailed(result) @@ -587,18 +602,6 @@ internal class ToolingApiServerImpl : IToolingApiServer { // Stop all daemons log.info("Stopping all Gradle Daemons...") DefaultGradleConnector.close() - - // After the daemons, not before. The exit is reported through - // handle.onExit().thenRun { scheduler.execute { ... } }, so a scheduler already - // shut down rejects it and the client never hears that the daemon it is - // plotting has gone. Through the delegate rather than the property: touching - // the property would build a watcher, and its scheduler, only to shut it down - // again on a server that never ran a build. - if (lazyDaemonWatcher.isInitialized()) { - log.info("Stopping the Gradle daemon watcher...") - runCatching { daemonWatcher.shutdown() } - .onFailure { log.warn("Could not stop the Gradle daemon watcher", it) } - } } // update the initialization flag before cancelling future @@ -609,6 +612,7 @@ internal class ToolingApiServerImpl : IToolingApiServer { log.info("Cancelling awaiting future...") Main.future?.cancel(true) + this.client = null this.buildCancellationToken = null this.lastInitParams = null this.lastWrapperDistribution = null @@ -616,12 +620,6 @@ internal class ToolingApiServerImpl : IToolingApiServer { // wait for connections to close connectionCloseFuture.get() - // After the wait, not before. Stopping the daemons is what makes the watcher report - // their exit, and that report goes through `client` -- cleared first, it was a silent - // no-op and the client's chart kept the daemon's last value. Best effort even so: the - // client's own channel is going away at the same time. - this.client = null - log.info("Shutdown request completed.") null } @@ -653,6 +651,18 @@ internal class ToolingApiServerImpl : IToolingApiServer { else -> UNKNOWN } + /** + * Marks the connection suspect when [error] says it may be dead, so the next build replaces it + * rather than reusing it. Only [executeTasks]'s build call reaches this; see the note there. + */ + private fun markSuspectIfConnectionFailure(error: Throwable) { + val failure = getTaskFailureType(error) + if (failure == CONNECTION_CLOSED || failure == CONNECTION_ERROR) { + log.warn("Marking the Gradle connection suspect after {}; the next build reconnects", failure) + connectionSuspect = true + } + } + private inline fun supplyAsync(crossinline action: () -> T): CompletableFuture = CompletableFuture.supplyAsync { action() @@ -677,19 +687,18 @@ internal class ToolingApiServerImpl : IToolingApiServer { crossinline action: () -> T, ): CompletableFuture = supplyAsync { - if (isBuildInProgress) { + if (!buildInProgress.compareAndSet(false, true)) { log.error("Cannot run build, build is already in progress!") throw IllegalStateException("Build is already in progress") } - isBuildInProgress = true daemonWatcher.onBuildStarted() try { action() } catch (error: Throwable) { onFailure(error) } finally { - isBuildInProgress = false + buildInProgress.set(false) } } diff --git a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt index 7f26a10f58..a846593702 100644 --- a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt +++ b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt @@ -380,7 +380,57 @@ class ToolingApiServerImplTest { assertThat(result.isSuccessful).isFalse() assertThat(result.failure).isEqualTo(TaskExecutionResult.Failure.CONNECTION_CLOSED) - // And this is the one site that marks the connection suspect. - assertThat(server.connectionSuspect).isTrue() + } + + @Test + fun `GIVEN a failure before the build runs THEN the connection is not condemned for it`() { + val (server) = mockkToolingServer() + every { server.isServerInitialized() } returns CompletableFuture.completedFuture(true) + + mockkObject(Main) + // A setup failure, not a build one. classifyTaskFailure reads any IllegalStateException as + // CONNECTION_CLOSED, so marking the connection from runBuild's whole-action catch condemned + // it over a throw from here -- and the next build's reconnect sends the running daemon + // StopWhenIdle, which is the cold start this ticket exists to prevent. + every { Main.checkGradleWrapper() } throws IllegalStateException("no wrapper") + + val result = + server + .executeTasks(TaskExecutionMessage(tasks = listOf("assembleDebug"), buildId = BuildId.Unknown)) + .get(5, TimeUnit.SECONDS) + + assertThat(result.isSuccessful).isFalse() + assertThat(server.connectionSuspect).isFalse() + } + + @Test + fun `GIVEN a build already running THEN a second is refused rather than sharing its connection`() { + // The refusal, not the atomicity. This passes against a read-then-write too -- the first + // build has set the flag long before the second reads it -- so it pins the behaviour and + // not the CAS. A genuine interleaving is not reproducible on demand, so the compareAndSet + // in runBuild is argued from the threading model rather than pinned here. + val (server) = mockkToolingServer() + every { server.isServerInitialized() } returns CompletableFuture.completedFuture(true) + + val started = java.util.concurrent.CountDownLatch(1) + val release = java.util.concurrent.CountDownLatch(1) + mockkObject(Main) + every { Main.checkGradleWrapper() } answers { + started.countDown() + release.await(5, TimeUnit.SECONDS) + throw IllegalStateException("done") + } + + val first = server.executeTasks(TaskExecutionMessage(tasks = listOf("a"), buildId = BuildId.Unknown)) + assertThat(started.await(5, TimeUnit.SECONDS)).isTrue() + + val second = server.executeTasks(TaskExecutionMessage(tasks = listOf("b"), buildId = BuildId.Unknown)) + val refused = runCatching { second.get(5, TimeUnit.SECONDS) }.exceptionOrNull() + + release.countDown() + first.get(5, TimeUnit.SECONDS) + + assertThat(refused).isNotNull() + assertThat(refused).hasCauseThat().hasMessageThat().contains("already in progress") } }