ADFA-5589: stop killing the warm Gradle daemon on every activity recreate - #1813
Draft
davidschachterADFA wants to merge 8 commits into
Draft
ADFA-5589: stop killing the warm Gradle daemon on every activity recreate#1813davidschachterADFA wants to merge 8 commits into
davidschachterADFA wants to merge 8 commits into
Conversation
…eate 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 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 Summary
WalkthroughThe tooling API now reuses an existing connector when project directory and Gradle distribution match. Tests cover equivalent parameters, differing connection details, and an uninitialized previous state. ChangesConnector Reuse
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Equivalent project initialization requests now retain the existing Gradle connector across activity recreation, avoiding unnecessary daemon shutdowns; the matching and non-matching cases are covered by tests. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
A rabbit checks the project path, Comment |
hal-eisen-adfa
marked this pull request as draft
September 9, 2026 02:43
Collaborator
|
Converting this to draft until the PR queue drains a bit more |
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 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
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 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
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 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
…emon-survives-recreate
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 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
…very 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 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
davidschachterADFA
added a commit
that referenced
this pull request
Sep 9, 2026
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 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
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 <[email protected]> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
Collaborator
Author
Merge hazard: #1812 and #1813 rewrite the same two functions, and one of the clashes is silentI merged these two branches locally twice today — once to retire a review finding about untested merged behaviour, once to build a combined APK — and hit the same traps both times. Recording them here so whoever lands these second doesn't rediscover them. Both PRs rewrite
The conflict git flagsThree hunks in
The clash git does not flag
notifyBuildFailure(result = BuildResult(tasks = ..., buildId = ..., durationMs = ...))
TaskExecutionResult(false, getTaskFailureType(error))It compiles, and the tests pass, because both signatures exist after the merge. It should be: TaskExecutionResult(false, notifyBuildFailure(message.buildId, message.tasks, start, error))Otherwise the failure is classified twice and reported through the older path. Nothing warns; only reading the function catches it. And a warning about resolving it by handMy own resolution left So: after merging these two, read Verified on the merged resultNot on either branch alone — that was the gap. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every activity recreate stopped the warm Gradle daemon, so the next build paid a full cold start.
The defect
initialize()decided whether it could reuse the open connector like this:InitializeProjectParamsdeclares noequalsand is not adata class, so==is 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 in production.Always-false means
forceConnect, which callsconnector.disconnect(), andGradleConnector.disconnect()sends the running daemonStopWhenIdle. The client re-initializes on every editor recreate outsideEditorActivityKt'sconfigChanges— a Dark/Light/Amoled switch, a locale change, a display-size change — so each one killed the daemon.The fix
Compare 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.
A
data classdoes not fix this, which is why that is not the change:buildIdis generated fresh per request, so value equality on the whole object stays false every time.Sibling sweep
InitializeProjectParamsis the only plain class among the tooling API's fourteen message types — the other thirteen are alreadydata classes, so nothing else in that package carries the same trap. The comparison site was also the only one;lastInitParamsis read in three other places and compared in none.Verified on device
Pixel 6 Pro, this branch's APK. Started a build, took the daemon's pid, then flipped the theme — the ticket's own repro.
cmd uimode night yescmd uimode night no(second recreate)Project is being reinitialized/Reusing connector instance...— the branch that had never executedStopWhenIdle, against one in this morning'sdaemon-6407.out.logThat last row is the strongest evidence: the stop request is not merely ineffective now, it is not sent.
Tests
Four cases on
describesSameConnection. The first fails without the fix — two params for one project are reported as different connections — and it also asserts that==says false for them, which states the defect directly. The other three bound it the other way: a different directory, a different distribution, and nothing initialized yet.describesSameConnectionis a separate function so it can be asserted at all. The rest ofcanReuseConnectorreads private state that only a real connect populates, and a test that stubs the connect never sets it — the first version of this test assertedforceConnectend to end and failed for that reason, not because the fix was wrong.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.