From a53feb44604a237ce478dc014f81d4dd08076063 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 9 Sep 2026 14:27:04 -0700 Subject: [PATCH 1/2] ADFA-5659: shut the Gradle daemon watcher down with the server GradleDaemonWatcher.shutdown() had no caller. Reported by hal-eisen-adfa on PR #1812, where the code is in the base commit rather than the diff -- it came in with ADFA-5514 via #1798, so it is live on stage. - 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 Two details that are easy to get wrong, both found in review of the version of this fix that rides ADFA-5589: It is called after DefaultGradleConnector.close(), not before. Stopping the daemons is what produces the exit, and the exit is reported through scheduler.execute { ... } -- a scheduler already shut down rejects it and merely logs, so the client never hears that the daemon it is plotting has gone. For the same reason the client is cleared after the wait rather than before it; best effort even then, since the client's own channel is going away at the same time. It goes through the lazy delegate rather than the property, or a server that never ran a build constructs a watcher, and its scheduler, purely to shut it down again. This is carried out of PR #1813, which is a draft while the review queue drains, so the fix does not wait on it. Tests: 9 in GradleDaemonWatcherTest, one new -- shutdown reaches scheduler.shutdownNow(). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../tooling/impl/ToolingApiServerImpl.kt | 35 +++++++++++++++---- .../tooling/impl/GradleDaemonWatcherTest.kt | 12 +++++++ 2 files changed, 40 insertions(+), 7 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..fe593fb972 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 @@ -361,12 +361,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) @@ -422,6 +425,19 @@ internal class ToolingApiServerImpl : IToolingApiServer { // Stop all daemons log.info("Stopping all Gradle Daemons...") DefaultGradleConnector.close() + + // After the daemons, not before. Stopping them is what produces the exit, and + // the exit is reported through handle.onExit().thenRun { scheduler.execute + // { ... } } -- a scheduler already shut down rejects it and merely logs, so the + // client never hears that the daemon it is plotting has gone. 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. + 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 @@ -432,13 +448,18 @@ internal class ToolingApiServerImpl : IToolingApiServer { log.info("Cancelling awaiting future...") Main.future?.cancel(true) - this.client = null this.buildCancellationToken = null this.lastInitParams = null // 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 } 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 f8b4fd0588c4548947ddfc3a6b9d5a9fd04f468d Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 9 Sep 2026 14:56:07 -0700 Subject: [PATCH 2/2] ADFA-5659: pin the fix at the caller, and stop claiming the exit is delivered Review of the first version found three problems with it, all mine. The only test passed against the unfixed code. GradleDaemonWatcher .shutdown() already read { scheduler.shutdownNow() } on stage -- what was missing was anything calling it -- so a test of the watcher in isolation pinned nothing, and deleting the new block in ToolingApiServerImpl left the suite green. CLAUDE.md asks for the opposite and I checked it on the sibling change and not on this one. There is now a seam (newDaemonWatcher, defaulted, the shape GradleDaemonWatcher itself uses for descendants and scheduler) and two tests at the caller: shutting the server down stops the watcher, and a server that never ran a build does not construct one just to stop it. Both fail against their own mutation. The "after the daemons, not before" ordering did not do what it claimed. The exit arrives through handle.onExit().thenRun { scheduler.execute { ... } }, and onExit completes on a process-reaper thread only once the OS has reaped the daemon -- strictly after DefaultGradleConnector.close() returns. There is no point in the sequence where the scheduler still accepts work and the daemon has already been reaped, so the report is not deliverable at shutdown under any ordering. The call goes back early, where it ends the scanning thread soonest and no late poll can report into a channel being torn down, and the comment says plainly that the shutdown-time exit is not delivered. Moving `client = null` after the wait made consequence #2 worse rather than better: it left the client non-null for the whole daemon-stopping window instead of none of it, so a late callback could reach a half-torn-down channel. It goes back where stage had it. GradleDaemonWatcher.shutdown() is now graceful then forceful -- a report already queued still runs, a poll wedged mid-scan cannot hold the process open. That part is a real improvement and is tested both ways. What remains true is consequence #1, which was always the defect: an unstopped watcher goes on scanning ProcessHandle.descendants() for up to a minute after the server is gone. Tests: 17 in the module, four new. Each fails against the mutation it is named for -- removing the call, dropping the isInitialized guard, reverting shutdown() to shutdownNow() alone. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../tooling/impl/GradleDaemonWatcher.kt | 26 ++++++++- .../tooling/impl/ToolingApiServerImpl.kt | 56 +++++++++++-------- .../tooling/impl/GradleDaemonWatcherTest.kt | 20 +++++-- .../tooling/impl/ToolingApiServerImplTest.kt | 35 ++++++++++++ 4 files changed, 109 insertions(+), 28 deletions(-) diff --git a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcher.kt b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcher.kt index d3bbe9bebd..2544dc5b42 100644 --- a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcher.kt +++ b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcher.kt @@ -124,8 +124,29 @@ internal class GradleDaemonWatcher( .onFailure { err -> log.warn("Failed to report exit of Gradle daemon {}", pid, err) } } + /** + * Stops the poller. + * + * Graceful first, so a report already queued -- an exit picked up moments before the server was + * told to stop -- still runs; then forcefully, so a poll asleep between attempts cannot hold the + * process open. [SHUTDOWN_GRACE_MS] is the bound: work is one `ProcessHandle.descendants()` + * scan, not a build, so a poll that has not finished in that long is wedged rather than busy. + * + * A report that has *not* been submitted yet is lost, and that is accepted: it arrives via + * onExit on a process-reaper thread once the OS reaps the daemon, which at shutdown is after + * everything here has run. + */ fun shutdown() { - scheduler.shutdownNow() + scheduler.shutdown() + val drained = + runCatching { scheduler.awaitTermination(SHUTDOWN_GRACE_MS, TimeUnit.MILLISECONDS) } + .getOrElse { + Thread.currentThread().interrupt() + false + } + if (!drained) { + scheduler.shutdownNow() + } } companion object { @@ -143,6 +164,9 @@ internal class GradleDaemonWatcher( private const val POLL_INTERVAL_MS = 500L + /** How long [shutdown] lets queued reports finish before it stops waiting. */ + const val SHUTDOWN_GRACE_MS = 250L + /** Bounded at roughly a minute, which is far longer than a daemon takes to come up. */ const val MAX_POLL_ATTEMPTS = 120 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 fe593fb972..6503626fa4 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 @@ -73,7 +73,10 @@ import kotlin.concurrent.withLock * * @author Akash Yadav */ -internal class ToolingApiServerImpl : IToolingApiServer { +internal class ToolingApiServerImpl( + private val newDaemonWatcher: (onStarted: (Int) -> Unit, onExited: (Int) -> Unit) -> GradleDaemonWatcher = + { onStarted, onExited -> GradleDaemonWatcher(onStarted = onStarted, onExited = onExited) }, +) : IToolingApiServer { private var client: IToolingApiClient? = null private var connector: GradleConnector? = null private var connection: ProjectConnection? = null @@ -363,9 +366,9 @@ internal class ToolingApiServerImpl : IToolingApiServer { */ private val lazyDaemonWatcher = lazy { - GradleDaemonWatcher( - onStarted = { pid -> client?.onGradleDaemonStarted(pid) }, - onExited = { pid -> client?.onGradleDaemonExited(pid) }, + newDaemonWatcher( + { pid -> client?.onGradleDaemonStarted(pid) }, + { pid -> client?.onGradleDaemonExited(pid) }, ) } @@ -410,6 +413,31 @@ internal class ToolingApiServerImpl : IToolingApiServer { buildCancellationToken?.cancel() buildCancellationToken = null + // Early, and deliberately not "late enough to report the daemon's exit". + // + // The leaked thread is the defect: unstopped, an in-flight poll chain goes on scanning + // ProcessHandle.descendants() for up to a minute after the server is gone. Stopping it + // here ends that at once, and means no later poll can report a daemon into an RPC + // channel that is being torn down. + // + // Delivering the shutdown-time exit was tried and does not work. That report arrives + // through handle.onExit().thenRun { scheduler.execute { ... } }, and onExit completes + // on a process-reaper thread only once the OS has reaped the daemon -- strictly after + // DefaultGradleConnector.close() returns. There is no point in this sequence where the + // scheduler is still accepting work *and* the daemon has already been reaped, so the + // report is not deliverable at shutdown whatever the ordering; keeping `client` alive + // for it only widens the window in which a half-torn-down channel can be written to. + // The client learns the daemon is gone when it reconnects, not from here. + // + // 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. + 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 @@ -425,19 +453,6 @@ internal class ToolingApiServerImpl : IToolingApiServer { // Stop all daemons log.info("Stopping all Gradle Daemons...") DefaultGradleConnector.close() - - // After the daemons, not before. Stopping them is what produces the exit, and - // the exit is reported through handle.onExit().thenRun { scheduler.execute - // { ... } } -- a scheduler already shut down rejects it and merely logs, so the - // client never hears that the daemon it is plotting has gone. 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. - 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 @@ -448,18 +463,13 @@ internal class ToolingApiServerImpl : IToolingApiServer { log.info("Cancelling awaiting future...") Main.future?.cancel(true) + this.client = null this.buildCancellationToken = null this.lastInitParams = null // 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 } 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 470d042112..266f343d4a 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 @@ -215,11 +215,23 @@ class GradleDaemonWatcherTest { } @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. + fun `shutdown drains what is queued before it stops waiting`() { + // Graceful first: a report queued moments before the server was told to stop still runs. val scheduler = mockk(relaxed = true) + every { scheduler.awaitTermination(any(), any()) } returns true + + watcher(scheduler = scheduler).shutdown() + + verify(exactly = 1) { scheduler.shutdown() } + verify(exactly = 1) { scheduler.awaitTermination(GradleDaemonWatcher.SHUTDOWN_GRACE_MS, TimeUnit.MILLISECONDS) } + verify(exactly = 0) { scheduler.shutdownNow() } + } + + @Test + fun `shutdown stops waiting on a poll that will not finish`() { + // And forcefully after the grace period, so a wedged scan cannot hold the process open. + val scheduler = mockk(relaxed = true) + every { scheduler.awaitTermination(any(), any()) } returns false watcher(scheduler = scheduler).shutdown() 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..7ec9b8e8bd 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 @@ -124,4 +124,39 @@ class ToolingApiServerImplTest { RootModelBuilder.build(initParams, any()) } } + + @Test + fun `shutting the server down stops the daemon watcher`() { + // The defect this PR exists to fix, pinned at the caller. The watcher's own shutdown() was + // already correct on stage -- what was missing was anything calling it, so a test of + // GradleDaemonWatcher.shutdown() in isolation passes against the unfixed server and pins + // nothing. Deleting the block in ToolingApiServerImpl.shutdown() has to fail a test. + val watcher = mockk(relaxed = true) + val server = ToolingApiServerImpl(newDaemonWatcher = { _, _ -> watcher }) + + // A build, to bring the watcher into being the way a session does: runBuild calls + // onBuildStarted, which is what initializes the lazy. + server.initialize(testInitParams()).get(5, TimeUnit.SECONDS) + server.shutdown().get(5, TimeUnit.SECONDS) + + verify(exactly = 1) { watcher.shutdown() } + } + + @Test + fun `a server that never ran a build does not build a watcher just to stop it`() { + // Through the lazy delegate, not the property: touching the property would construct a + // watcher, and its scheduler thread, only to shut it down again. + var built = 0 + val server = + ToolingApiServerImpl( + newDaemonWatcher = { _, _ -> + built++ + mockk(relaxed = true) + }, + ) + + server.shutdown().get(5, TimeUnit.SECONDS) + + assertThat(built).isEqualTo(0) + } }