From 49a6c7d632add2d873382015ae4ae0dddd13e675 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 16:42:50 -0700 Subject: [PATCH 1/5] ADFA-5542: say which build a cancel belongs to The listener held a bare flag meaning "a cancel happened recently", and the only thing tying it to the build it belonged to was the order two main-thread runnables happened to run in. onBuildCancelRequested is raised on the UI thread and so runs inline; prepareBuild is raised from the build's own thread and so is posted. A cancel landing after that post and before it ran was cleared by it, and the build the user stopped was annotated as a failure -- which is the one thing BUILD_CANCELLED exists to prevent. Fixing the order would only move the window, so the cancel is keyed to a build instead. BuildInfo and BuildResult already carry a BuildId; the listener interface was dropping it one line from where it was needed. onBuildCancelRequested, onBuildSuccessful and onBuildFailed now carry the id, the service remembers which build is running, and the listener compares rather than sequences. That subsumes the other flag too. annotatedBuild existed for the same missing information -- the outcome callbacks are handed the server's task list, not the one prepareBuild saw, so the listener kept a flag to know whether the finish it was looking at belonged to the start it drew. Both are now nullable ids, and neither needs clearing per build: an id left from a build whose outcome never arrived cannot match the next build's, so prepareBuild no longer resets anything. The interface has no defaulted members (ADFA-5509), so every widening is a compile error for both implementers rather than a silent drop; the wrapper test now also asserts the id survives the forward, since a wrapper that passed the call and dropped the argument would be that same defect one layer in with nothing to complain about it. The decision is extracted as outcomeKind so it can be asserted without a live activity, matching isAnnotated beside it. Restoring the clear prepareBuild used to do fails the new test, expected BUILD_CANCELLED but was BUILD_FAILED, and fails nothing else. Not reproduced on device: the Stop control only appears after a 150ms delayed menu invalidation queued behind prepareBuild, so the window is not reachable by hand. It is reachable in a test, which is what the regression case drives. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../handlers/EditorBuildEventListener.kt | 92 +++++++++++++------ .../services/builder/GradleBuildService.kt | 73 ++++++++++++--- .../EditorBuildEventListenerAnnotationTest.kt | 63 +++++++++---- .../GradleBuildServiceListenerWrapperTest.kt | 26 +++++- 4 files changed, 189 insertions(+), 65 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt index c40e68d7b2..cd9fb59616 100644 --- a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt +++ b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt @@ -26,6 +26,7 @@ import com.itsaky.androidide.projects.builder.BuildResult import com.itsaky.androidide.projects.builder.LaunchResult import com.itsaky.androidide.resources.R.string import com.itsaky.androidide.services.builder.GradleBuildService +import com.itsaky.androidide.tooling.api.messages.BuildId import com.itsaky.androidide.tooling.api.messages.result.BuildInfo import com.itsaky.androidide.tooling.events.ProgressEvent import com.itsaky.androidide.tooling.events.configuration.ProjectConfigurationStartEvent @@ -50,14 +51,22 @@ class EditorBuildEventListener : GradleBuildService.EventListener { private var lastOutputTimeMs: Long = SystemClock.elapsedRealtime() /** - * Set when the user asks for the running build to stop, so [onBuildFailed] can tell a cancel - * from a real failure. Cleared as each build is prepared. + * The build the user asked to stop, so [onBuildFailed] can tell a cancel from a real failure. + * + * A build id and not a flag. A flag said only "a cancel happened recently", and the one thing + * tying it to the build it belonged to was the order two main-thread runnables happened to run + * in: [onBuildCancelRequested] is raised on the UI thread and so runs inline, while + * [prepareBuild] is raised from the build's own thread and so is posted. A cancel arriving + * after that post and before it ran was cleared by it, and the build the user stopped was + * annotated as a failure (ADFA-5542). Identity does not depend on that order, and there is + * nothing to clear per build: an id left over from a build whose outcome never arrived cannot + * match the next build's. */ @VisibleForTesting - internal var cancelRequested = false + internal var cancelledBuildId: BuildId? = null /** - * Whether the build now running drew a "Build started" marker. + * The build that drew a "Build started" marker, or null if the one running drew none. * * The outcome callbacks used to decide for themselves, from the task list they are handed -- * a different list from the one prepareBuild sees. If those two ever disagreed the chart got @@ -65,7 +74,7 @@ class EditorBuildEventListener : GradleBuildService.EventListener { * exists to avoid. The build that started decides, and its outcome follows. */ @VisibleForTesting - internal var annotatedBuild = false + internal var annotatedBuildId: BuildId? = null private var enabled = true private var activityReference: WeakReference = WeakReference(null) @@ -99,13 +108,6 @@ class EditorBuildEventListener : GradleBuildService.EventListener { } override fun prepareBuild(buildInfo: BuildInfo) { - // Before the activity check, not after: this listener outlives any one activity, so a - // build whose outcome arrived with none attached would otherwise leave both flags set for - // the next build to inherit -- a stale cancel mislabelling a real failure, or a stale - // pairing drawing a finish for a build that never started. - cancelRequested = false - annotatedBuild = false - val act = checkActivity("prepareBuild") ?: return // A project sync runs through the same callbacks with no tasks, so annotating every @@ -115,7 +117,7 @@ class EditorBuildEventListener : GradleBuildService.EventListener { // The outcome callbacks are handed their own task list, which is not this one. Recorded // here so the pair is decided once, by the build that started. if (buildInfo.tasks.isNotEmpty()) { - annotatedBuild = true + annotatedBuildId = buildInfo.buildId act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_STARTED) } @@ -143,18 +145,50 @@ class EditorBuildEventListener : GradleBuildService.EventListener { } } + /** + * Drops what is held for [buildId] now that its outcome has been reported. + * + * Only for that build: another id in either field belongs to a build whose outcome has not + * arrived, and clearing it would lose the pairing or the cancel that build is owed. + */ + private fun forget(buildId: BuildId) { + if (cancelledBuildId == buildId) { + cancelledBuildId = null + } + if (annotatedBuildId == buildId) { + annotatedBuildId = null + } + } + + /** + * Whether the failure of [buildId] is the user's own cancel or a real failure (ADFA-5542). + * + * Separated from [onBuildFailed] so the decision can be tested: that method needs a live + * activity before it reaches this point, and returns early without one. + */ + @VisibleForTesting + internal fun outcomeKind(buildId: BuildId): MetricsAnnotationStore.Kind = + if (cancelledBuildId == buildId) { + MetricsAnnotationStore.Kind.BUILD_CANCELLED + } else { + MetricsAnnotationStore.Kind.BUILD_FAILED + } + private fun resetBuildTimers() { buildStartTimeMs = System.currentTimeMillis() lastOutputTimeMs = SystemClock.elapsedRealtime() } - override fun onBuildSuccessful(tasks: List) { + override fun onBuildSuccessful( + buildId: BuildId, + tasks: List, + ) { val act = checkActivity("onBuildSuccessful") ?: return - if (annotatedBuild) { + if (annotatedBuildId == buildId) { act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_FINISHED) } - annotatedBuild = false + forget(buildId) pluginBuildService?.notifyBuildFinished() @@ -183,8 +217,12 @@ class EditorBuildEventListener : GradleBuildService.EventListener { lastStatusLine = "" } - override fun onBuildCancelRequested() { - cancelRequested = true + override fun onBuildCancelRequested(buildId: BuildId?) { + // A cancel with no build running names nothing to attribute it to. Leaving the id already + // held alone keeps a build still finishing from being relabelled by it. + if (buildId != null) { + cancelledBuildId = buildId + } } override fun onProgressEvent(event: ProgressEvent) { @@ -212,22 +250,18 @@ class EditorBuildEventListener : GradleBuildService.EventListener { @VisibleForTesting internal fun isAnnotated(event: ProgressEvent): Boolean = event is TaskStartEvent || event is TaskFinishEvent - override fun onBuildFailed(tasks: List) { + override fun onBuildFailed( + buildId: BuildId, + tasks: List, + ) { val act = checkActivity("onBuildFailed") ?: return - if (annotatedBuild) { + if (annotatedBuildId == buildId) { // A build the user stopped arrives through this same callback. Marking it as a failure // would report their own deliberate action back to them in the error colour. - act.recordBuildAnnotation( - if (cancelRequested) { - MetricsAnnotationStore.Kind.BUILD_CANCELLED - } else { - MetricsAnnotationStore.Kind.BUILD_FAILED - }, - ) + act.recordBuildAnnotation(outcomeKind(buildId)) } - annotatedBuild = false - cancelRequested = false + forget(buildId) analyzeCurrentFile() GeneralPreferences.isFirstBuild = false diff --git a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt index 437698d64d..4062a25a01 100644 --- a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt +++ b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt @@ -129,6 +129,16 @@ class GradleBuildService : private val buildSessionId = UUID.randomUUID().toString() private val buildId = AtomicLong(0) + /** + * The build a cancel request would belong to, or null when none is running. + * + * The listener cannot work this out from the order it is called in (ADFA-5542), so it is told, + * and this is what it is told. Written from the build's own thread in [prepareBuild] and read + * from the UI thread by [cancelCurrentBuild], hence volatile. + */ + @Volatile + private var runningBuildId: BuildId? = null + @Volatile private var tuningConfig: GradleTuningConfig? = null @@ -181,24 +191,30 @@ class GradleBuildService : null } else { object : EventListener { - override fun onBuildCancelRequested() { - runOnUiThread { listener.onBuildCancelRequested() } + override fun onBuildCancelRequested(buildId: BuildId?) { + runOnUiThread { listener.onBuildCancelRequested(buildId) } } override fun prepareBuild(buildInfo: BuildInfo) { runOnUiThread { listener.prepareBuild(buildInfo) } } - override fun onBuildSuccessful(tasks: List) { - runOnUiThread { listener.onBuildSuccessful(tasks) } + override fun onBuildSuccessful( + buildId: BuildId, + tasks: List, + ) { + runOnUiThread { listener.onBuildSuccessful(buildId, tasks) } } override fun onProgressEvent(event: ProgressEvent) { runOnUiThread { listener.onProgressEvent(event) } } - override fun onBuildFailed(tasks: List) { - runOnUiThread { listener.onBuildFailed(tasks) } + override fun onBuildFailed( + buildId: BuildId, + tasks: List, + ) { + runOnUiThread { listener.onBuildFailed(buildId, tasks) } } override fun onOutput(line: String?) { @@ -387,6 +403,12 @@ class GradleBuildService : override fun prepareBuild(buildInfo: BuildInfo): CompletableFuture = CompletableFuture.supplyAsync { + // The server raises this only for a build that really started, so a second request + // rejected as already-in-progress cannot take the running build's name off the cancel. + // It is also well before the editor is told the build began, and the Stop control + // follows from that, so a cancel can never arrive with this unset. + runningBuildId = buildInfo.buildId + updateNotification(getString(R.string.build_status_in_progress), true) val projectPath = ProjectManagerImpl.getInstance().projectDirPath ?: "unknown" @@ -457,20 +479,26 @@ class GradleBuildService : updateNotification(getString(R.string.build_status_sucess), false) dispatchBuildResult(result, true) - eventListener?.onBuildSuccessful(result.tasks) + eventListener?.onBuildSuccessful(result.buildId, result.tasks) } override fun onBuildFailed(result: BuildResult) { updateNotification(getString(R.string.build_status_failed), false) dispatchBuildResult(result, false) - eventListener?.onBuildFailed(result.tasks) + eventListener?.onBuildFailed(result.buildId, result.tasks) } private fun dispatchBuildResult( result: BuildResult, isSuccess: Boolean, ) { + // Only if it is still this build's: a build whose result never arrived leaves its id here, + // and clearing that on someone else's outcome would drop the name off a live cancel. + if (runningBuildId == result.buildId) { + runningBuildId = null + } + val buildType = getBuildType(result.tasks) analyticsManager.trackBuildCompleted( metric = @@ -666,8 +694,9 @@ class GradleBuildService : override fun cancelCurrentBuild(): CompletableFuture { checkServerStarted() // Before delegating: the cancellation surfaces as a build failure, and the listener needs - // to know it was asked for rather than reporting the user's own action as an error. - eventListener?.onBuildCancelRequested() + // to know it was asked for rather than reporting the user's own action as an error. It is + // told which build, because it cannot infer that from when this arrives (ADFA-5542). + eventListener?.onBuildCancelRequested(runningBuildId) return server!!.cancelCurrentBuild() } @@ -826,8 +855,13 @@ class GradleBuildService : * then quietly inherited the no-op instead of passing it on -- so the cancel never reached * the real listener, and a build the user stopped went on being annotated as a failure. A * member with no default cannot be forgotten by a wrapper; the compiler asks for it. + * + * @param buildId The build being stopped, or null if none was running. Carried because a + * listener cannot tell from arrival order which build a cancel belongs to: this call is + * made on the UI thread and runs inline, while [prepareBuild] is made from the build's + * own thread and is posted (ADFA-5542). */ - fun onBuildCancelRequested() + fun onBuildCancelRequested(buildId: BuildId?) /** * Called just before a build is started. @@ -840,10 +874,15 @@ class GradleBuildService : /** * Called when a build is successful. * + * @param buildId The build that succeeded. [tasks] is the server's own list and not the one + * [prepareBuild] was given, so this is the only thing that names the build. * @param tasks The tasks that were run. * @see IToolingApiClient.onBuildSuccessful */ - fun onBuildSuccessful(tasks: List) + fun onBuildSuccessful( + buildId: BuildId, + tasks: List, + ) /** * Called when a progress event is received from the Tooling API server. @@ -855,10 +894,18 @@ class GradleBuildService : /** * Called when a build fails. * + * A build the user cancelled arrives here too; compare [buildId] with the one + * [onBuildCancelRequested] named to tell the two apart. + * + * @param buildId The build that failed. [tasks] is the server's own list and not the one + * [prepareBuild] was given, so this is the only thing that names the build. * @param tasks The tasks that were run. * @see IToolingApiClient.onBuildFailed */ - fun onBuildFailed(tasks: List) + fun onBuildFailed( + buildId: BuildId, + tasks: List, + ) /** * Called when the output line is received. diff --git a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt index 5fe152155e..466bdf8b39 100644 --- a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt +++ b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt @@ -19,6 +19,7 @@ package com.itsaky.androidide.handlers import com.google.common.truth.Truth.assertThat import com.itsaky.androidide.tooling.api.messages.BuildId +import com.itsaky.androidide.tooling.api.messages.BuildRunType import com.itsaky.androidide.tooling.api.messages.result.BuildInfo import com.itsaky.androidide.tooling.events.ProgressEvent import com.itsaky.androidide.tooling.events.internal.DefaultOperationDescriptor @@ -28,20 +29,25 @@ import com.itsaky.androidide.tooling.events.task.TaskFinishEvent import com.itsaky.androidide.tooling.events.task.TaskOperationDescriptor import com.itsaky.androidide.tooling.events.task.TaskStartEvent import com.itsaky.androidide.tooling.model.PluginIdentifier +import com.itsaky.androidide.utils.MetricsAnnotationStore import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner /** - * Which Gradle progress events the metrics charts annotate (ADFA-5486). + * What the metrics charts annotate, and which build each annotation belongs to. * - * Task starts and stops, and nothing else. Asserted against the predicate rather than through - * `onProgressEvent`, which needs a live activity before it gets this far. + * Two decisions, both asserted against the predicate that makes them rather than through the + * callback that acts on it -- those need a live activity before they get this far. Which progress + * events are marked at all (ADFA-5486), and whether a build that failed was really the user + * stopping it (ADFA-5542). */ @RunWith(RobolectricTestRunner::class) class EditorBuildEventListenerAnnotationTest { private val listener = EditorBuildEventListener() + private fun buildId(id: Long) = BuildId(buildSessionId = "session", buildId = id, runType = BuildRunType.TaskRun) + private fun taskDescriptor() = TaskOperationDescriptor( dependencies = emptySet(), @@ -74,28 +80,45 @@ class EditorBuildEventListenerAnnotationTest { ) @Test - fun `preparing a build clears a stale cancel, even with no activity attached`() { - listener.cancelRequested = true - - // No activity is attached here, so prepareBuild returns early -- which is the point. This - // listener outlives any one activity, and a cancel whose onBuildFailed arrived without one - // would otherwise leave the flag set for the next build to inherit and be mislabelled. - listener.prepareBuild(BuildInfo(BuildId.Unknown, listOf(":app:assembleDebug"))) - - assertThat(listener.cancelRequested).isFalse() + fun `a cancel that lands before its build is prepared still marks that build cancelled`() { + // The interleaving ADFA-5542 is about, and the one the main thread can really produce: + // onBuildCancelRequested is raised on the UI thread and runs inline, prepareBuild is + // raised from the build's own thread and is posted, so the cancel can overtake it. When + // the listener held a bare flag, prepareBuild cleared it and the build the user stopped + // was reported back to them as a failure. + listener.onBuildCancelRequested(buildId(7)) + listener.prepareBuild(BuildInfo(buildId(7), listOf(":app:assembleDebug"))) + + assertThat(listener.outcomeKind(buildId(7))) + .isEqualTo(MetricsAnnotationStore.Kind.BUILD_CANCELLED) } @Test - fun `preparing a build clears a stale pairing`() { - listener.annotatedBuild = true + fun `a cancel is not inherited by the next build`() { + // The other half of keying to a build rather than to a moment. This listener outlives any + // one activity, so a cancel whose outcome never arrived stays held -- and must not relabel + // the next build's genuine failure. + listener.onBuildCancelRequested(buildId(7)) + + assertThat(listener.outcomeKind(buildId(8))) + .isEqualTo(MetricsAnnotationStore.Kind.BUILD_FAILED) + } - // The flag means "a start marker was drawn for the build now running", so a new build - // must not inherit it: the outcome callbacks read it to decide whether to draw the other - // half of the pair, and they are handed a different task list from this one. Cleared - // before the activity check for the same reason as the cancel flag. - listener.prepareBuild(BuildInfo(BuildId.Unknown, listOf(":app:assembleDebug"))) + @Test + fun `a build nobody stopped is a failure`() { + assertThat(listener.outcomeKind(buildId(7))) + .isEqualTo(MetricsAnnotationStore.Kind.BUILD_FAILED) + } - assertThat(listener.annotatedBuild).isFalse() + @Test + fun `a cancel naming no build leaves the one already held alone`() { + // cancelCurrentBuild passes null when nothing is running. Taking that as "forget the + // cancel" would lose the attribution for a build still finishing. + listener.onBuildCancelRequested(buildId(7)) + listener.onBuildCancelRequested(null) + + assertThat(listener.outcomeKind(buildId(7))) + .isEqualTo(MetricsAnnotationStore.Kind.BUILD_CANCELLED) } @Test diff --git a/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt index d515017ffa..a8d8ca2f33 100644 --- a/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt @@ -19,6 +19,7 @@ package com.itsaky.androidide.services.builder import com.google.common.truth.Truth.assertThat import com.itsaky.androidide.services.builder.GradleBuildService.EventListener +import com.itsaky.androidide.tooling.api.messages.BuildId import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -40,12 +41,15 @@ class GradleBuildServiceListenerWrapperTest { private class Recorder { val calls = mutableListOf() + var lastArgs: List = emptyList() + val listener: EventListener = Proxy.newProxyInstance( EventListener::class.java.classLoader, arrayOf(EventListener::class.java), - ) { _, method, _ -> + ) { _, method, args -> calls += method.name + lastArgs = args.orEmpty().toList() null } as EventListener } @@ -71,14 +75,30 @@ class GradleBuildServiceListenerWrapperTest { } @Test - fun `a cancel request reaches the listener`() { + fun `a cancel request reaches the listener, naming its build`() { val recorder = Recorder() val wrapped = GradleBuildService.wrap(recorder.listener)!! - wrapped.onBuildCancelRequested() + wrapped.onBuildCancelRequested(BuildId.Unknown) // The one this went wrong on, kept as its own case so the reason is legible in a report. assertThat(recorder.calls).containsExactly("onBuildCancelRequested") + + // The id is the whole of what makes a cancel attributable (ADFA-5542). A wrapper that + // forwarded the call and dropped the argument would be the same defect one layer in, and + // no signature would complain about it. + assertThat(recorder.lastArgs).containsExactly(BuildId.Unknown) + } + + @Test + fun `an outcome reaches the listener with the build it belongs to`() { + val recorder = Recorder() + val wrapped = GradleBuildService.wrap(recorder.listener)!! + + wrapped.onBuildFailed(BuildId.Unknown, listOf(":app:assembleDebug")) + + assertThat(recorder.calls).containsExactly("onBuildFailed") + assertThat(recorder.lastArgs).containsExactly(BuildId.Unknown, listOf(":app:assembleDebug")).inOrder() } @Test From c6d6393d56b0e3797c0ee5b3ab4023b740735c61 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 17:40:04 -0700 Subject: [PATCH 2/5] ADFA-5542: route the server's own verdict instead of guessing it Replaces the approach in the commit before this one, which had the client reconstruct whether a failure was a cancel by remembering which build was running and matching ids against it. The review found the premise wrong: the Stop control goes live at build submission, not when the server calls back prepareBuild, so the window was still open and now failed silently -- runningBuildId was null and the cancel was dropped by the very guard meant to protect it. It found the better answer too. The tooling server already knows. It catches the throwable Gradle raised and getTaskFailureType maps a BuildCancelledException to Failure.BUILD_CANCELLED -- one classification, no threading, no ambiguity -- and then hands that answer only to the caller of executeTasks while the BuildResult it notifies the client with carries tasks, an id and a duration. The question was answered exactly, one line from where it was needed, and thrown away. So BuildResult carries the failure now, the same value the return path gets, from one call. Both server failure sites classify once and use it twice. The listener reads it. That deletes the whole of the previous attempt: runningBuildId, the volatile it needed and its non-atomic compare-and-clear, cancelledBuildId, forget(), the build id on the outcome callbacks, and onBuildCancelRequested itself, which existed only to tell the listener something it can now be shown. annotatedBuild goes back to a boolean cleared per build, above the activity check, which is where it was before and where it belongs -- the reason to key it to a build was to avoid a reset that was never the problem. Net 30 lines shorter than the code it replaces, in a change that fixes more. ADFA-5509's abstractness invariant on EventListener stays and still guards every remaining member. The wrapper test that covered the removed callback now covers the failure reason instead, for the same reason it existed: a wrapper that forwarded the call and dropped the argument would be that defect one layer in, with no signature to complain. The regression test is on the server, because that is where the fix is: a build that fails with BuildCancelledException must report BUILD_CANCELLED to the client and not only to its caller. Dropping the field from the notified BuildResult fails it, and nothing else. Also fixes the compile of :subprojects:tooling-api-impl's tests, which have been broken since ADFA-2784 added buildId to InitializeProjectParams: testInitParams never passed it. It is broken on stage too. No workflow runs :subprojects: tests, which is why nothing said so -- worth a ticket of its own. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../handlers/EditorBuildEventListener.kt | 78 ++++++------------ .../services/builder/GradleBuildService.kt | 80 +++---------------- .../EditorBuildEventListenerAnnotationTest.kt | 69 ++++++++-------- .../GradleBuildServiceListenerWrapperTest.kt | 47 +++++------ .../tooling/impl/ToolingApiServerImpl.kt | 10 ++- .../tooling/impl/ToolingApiServerImplTest.kt | 58 ++++++++++++-- .../api/messages/result/BuildResult.kt | 10 +++ 7 files changed, 161 insertions(+), 191 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt index cd9fb59616..4f4196f731 100644 --- a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt +++ b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt @@ -26,8 +26,8 @@ import com.itsaky.androidide.projects.builder.BuildResult import com.itsaky.androidide.projects.builder.LaunchResult import com.itsaky.androidide.resources.R.string import com.itsaky.androidide.services.builder.GradleBuildService -import com.itsaky.androidide.tooling.api.messages.BuildId import com.itsaky.androidide.tooling.api.messages.result.BuildInfo +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import com.itsaky.androidide.tooling.events.ProgressEvent import com.itsaky.androidide.tooling.events.configuration.ProjectConfigurationStartEvent import com.itsaky.androidide.tooling.events.task.TaskFinishEvent @@ -51,22 +51,7 @@ class EditorBuildEventListener : GradleBuildService.EventListener { private var lastOutputTimeMs: Long = SystemClock.elapsedRealtime() /** - * The build the user asked to stop, so [onBuildFailed] can tell a cancel from a real failure. - * - * A build id and not a flag. A flag said only "a cancel happened recently", and the one thing - * tying it to the build it belonged to was the order two main-thread runnables happened to run - * in: [onBuildCancelRequested] is raised on the UI thread and so runs inline, while - * [prepareBuild] is raised from the build's own thread and so is posted. A cancel arriving - * after that post and before it ran was cleared by it, and the build the user stopped was - * annotated as a failure (ADFA-5542). Identity does not depend on that order, and there is - * nothing to clear per build: an id left over from a build whose outcome never arrived cannot - * match the next build's. - */ - @VisibleForTesting - internal var cancelledBuildId: BuildId? = null - - /** - * The build that drew a "Build started" marker, or null if the one running drew none. + * Whether the build now running drew a "Build started" marker. * * The outcome callbacks used to decide for themselves, from the task list they are handed -- * a different list from the one prepareBuild sees. If those two ever disagreed the chart got @@ -74,7 +59,7 @@ class EditorBuildEventListener : GradleBuildService.EventListener { * exists to avoid. The build that started decides, and its outcome follows. */ @VisibleForTesting - internal var annotatedBuildId: BuildId? = null + internal var annotatedBuild = false private var enabled = true private var activityReference: WeakReference = WeakReference(null) @@ -108,6 +93,11 @@ class EditorBuildEventListener : GradleBuildService.EventListener { } override fun prepareBuild(buildInfo: BuildInfo) { + // Before the activity check, not after: this listener outlives any one activity, so a + // build whose outcome arrived with none attached would otherwise leave the flag set for + // the next build to inherit and draw a finish for a build that never started. + annotatedBuild = false + val act = checkActivity("prepareBuild") ?: return // A project sync runs through the same callbacks with no tasks, so annotating every @@ -117,7 +107,7 @@ class EditorBuildEventListener : GradleBuildService.EventListener { // The outcome callbacks are handed their own task list, which is not this one. Recorded // here so the pair is decided once, by the build that started. if (buildInfo.tasks.isNotEmpty()) { - annotatedBuildId = buildInfo.buildId + annotatedBuild = true act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_STARTED) } @@ -146,29 +136,20 @@ class EditorBuildEventListener : GradleBuildService.EventListener { } /** - * Drops what is held for [buildId] now that its outcome has been reported. + * Which marker a failed build gets: the user's own cancel, or a real failure (ADFA-5542). * - * Only for that build: another id in either field belongs to a build whose outcome has not - * arrived, and clearing it would lose the pairing or the cancel that build is owed. - */ - private fun forget(buildId: BuildId) { - if (cancelledBuildId == buildId) { - cancelledBuildId = null - } - if (annotatedBuildId == buildId) { - annotatedBuildId = null - } - } - - /** - * Whether the failure of [buildId] is the user's own cancel or a real failure (ADFA-5542). + * [failure] is the server's own classification of the throwable Gradle raised. The listener + * used to answer this from a flag it set when the cancel was requested, which meant deciding + * from the order two main-thread runnables happened to run in -- and a cancel that overtook + * [prepareBuild] was cleared by it, so the build the user stopped was reported back to them as + * an error. * * Separated from [onBuildFailed] so the decision can be tested: that method needs a live * activity before it reaches this point, and returns early without one. */ @VisibleForTesting - internal fun outcomeKind(buildId: BuildId): MetricsAnnotationStore.Kind = - if (cancelledBuildId == buildId) { + internal fun outcomeKind(failure: TaskExecutionResult.Failure?): MetricsAnnotationStore.Kind = + if (failure == TaskExecutionResult.Failure.BUILD_CANCELLED) { MetricsAnnotationStore.Kind.BUILD_CANCELLED } else { MetricsAnnotationStore.Kind.BUILD_FAILED @@ -179,16 +160,13 @@ class EditorBuildEventListener : GradleBuildService.EventListener { lastOutputTimeMs = SystemClock.elapsedRealtime() } - override fun onBuildSuccessful( - buildId: BuildId, - tasks: List, - ) { + override fun onBuildSuccessful(tasks: List) { val act = checkActivity("onBuildSuccessful") ?: return - if (annotatedBuildId == buildId) { + if (annotatedBuild) { act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_FINISHED) } - forget(buildId) + annotatedBuild = false pluginBuildService?.notifyBuildFinished() @@ -217,14 +195,6 @@ class EditorBuildEventListener : GradleBuildService.EventListener { lastStatusLine = "" } - override fun onBuildCancelRequested(buildId: BuildId?) { - // A cancel with no build running names nothing to attribute it to. Leaving the id already - // held alone keeps a build still finishing from being relabelled by it. - if (buildId != null) { - cancelledBuildId = buildId - } - } - override fun onProgressEvent(event: ProgressEvent) { val act = checkActivity("onProgressEvent") ?: return @@ -251,17 +221,17 @@ class EditorBuildEventListener : GradleBuildService.EventListener { internal fun isAnnotated(event: ProgressEvent): Boolean = event is TaskStartEvent || event is TaskFinishEvent override fun onBuildFailed( - buildId: BuildId, tasks: List, + failure: TaskExecutionResult.Failure?, ) { val act = checkActivity("onBuildFailed") ?: return - if (annotatedBuildId == buildId) { + if (annotatedBuild) { // A build the user stopped arrives through this same callback. Marking it as a failure // would report their own deliberate action back to them in the error colour. - act.recordBuildAnnotation(outcomeKind(buildId)) + act.recordBuildAnnotation(outcomeKind(failure)) } - forget(buildId) + annotatedBuild = false analyzeCurrentFile() GeneralPreferences.isFirstBuild = false diff --git a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt index 4062a25a01..a2809db546 100644 --- a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt +++ b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt @@ -129,16 +129,6 @@ class GradleBuildService : private val buildSessionId = UUID.randomUUID().toString() private val buildId = AtomicLong(0) - /** - * The build a cancel request would belong to, or null when none is running. - * - * The listener cannot work this out from the order it is called in (ADFA-5542), so it is told, - * and this is what it is told. Written from the build's own thread in [prepareBuild] and read - * from the UI thread by [cancelCurrentBuild], hence volatile. - */ - @Volatile - private var runningBuildId: BuildId? = null - @Volatile private var tuningConfig: GradleTuningConfig? = null @@ -191,19 +181,12 @@ class GradleBuildService : null } else { object : EventListener { - override fun onBuildCancelRequested(buildId: BuildId?) { - runOnUiThread { listener.onBuildCancelRequested(buildId) } - } - override fun prepareBuild(buildInfo: BuildInfo) { runOnUiThread { listener.prepareBuild(buildInfo) } } - override fun onBuildSuccessful( - buildId: BuildId, - tasks: List, - ) { - runOnUiThread { listener.onBuildSuccessful(buildId, tasks) } + override fun onBuildSuccessful(tasks: List) { + runOnUiThread { listener.onBuildSuccessful(tasks) } } override fun onProgressEvent(event: ProgressEvent) { @@ -211,10 +194,10 @@ class GradleBuildService : } override fun onBuildFailed( - buildId: BuildId, tasks: List, + failure: TaskExecutionResult.Failure?, ) { - runOnUiThread { listener.onBuildFailed(buildId, tasks) } + runOnUiThread { listener.onBuildFailed(tasks, failure) } } override fun onOutput(line: String?) { @@ -403,12 +386,6 @@ class GradleBuildService : override fun prepareBuild(buildInfo: BuildInfo): CompletableFuture = CompletableFuture.supplyAsync { - // The server raises this only for a build that really started, so a second request - // rejected as already-in-progress cannot take the running build's name off the cancel. - // It is also well before the editor is told the build began, and the Stop control - // follows from that, so a cancel can never arrive with this unset. - runningBuildId = buildInfo.buildId - updateNotification(getString(R.string.build_status_in_progress), true) val projectPath = ProjectManagerImpl.getInstance().projectDirPath ?: "unknown" @@ -479,26 +456,20 @@ class GradleBuildService : updateNotification(getString(R.string.build_status_sucess), false) dispatchBuildResult(result, true) - eventListener?.onBuildSuccessful(result.buildId, result.tasks) + eventListener?.onBuildSuccessful(result.tasks) } override fun onBuildFailed(result: BuildResult) { updateNotification(getString(R.string.build_status_failed), false) dispatchBuildResult(result, false) - eventListener?.onBuildFailed(result.buildId, result.tasks) + eventListener?.onBuildFailed(result.tasks, result.failure) } private fun dispatchBuildResult( result: BuildResult, isSuccess: Boolean, ) { - // Only if it is still this build's: a build whose result never arrived leaves its id here, - // and clearing that on someone else's outcome would drop the name off a live cancel. - if (runningBuildId == result.buildId) { - runningBuildId = null - } - val buildType = getBuildType(result.tasks) analyticsManager.trackBuildCompleted( metric = @@ -693,10 +664,6 @@ class GradleBuildService : override fun cancelCurrentBuild(): CompletableFuture { checkServerStarted() - // Before delegating: the cancellation surfaces as a build failure, and the listener needs - // to know it was asked for rather than reporting the user's own action as an error. It is - // told which build, because it cannot infer that from when this arrives (ADFA-5542). - eventListener?.onBuildCancelRequested(runningBuildId) return server!!.cancelCurrentBuild() } @@ -845,24 +812,6 @@ class GradleBuildService : /** Handles events received from a Gradle build. */ interface EventListener { - /** - * Called when the user asks for the running build to stop. - * - * The tooling API reports a cancelled build through [onBuildFailed], so a listener that - * wants to tell the two apart has to be told here. - * - * Deliberately not defaulted. It was, and the forwarding wrapper in [GradleBuildService] - * then quietly inherited the no-op instead of passing it on -- so the cancel never reached - * the real listener, and a build the user stopped went on being annotated as a failure. A - * member with no default cannot be forgotten by a wrapper; the compiler asks for it. - * - * @param buildId The build being stopped, or null if none was running. Carried because a - * listener cannot tell from arrival order which build a cancel belongs to: this call is - * made on the UI thread and runs inline, while [prepareBuild] is made from the build's - * own thread and is posted (ADFA-5542). - */ - fun onBuildCancelRequested(buildId: BuildId?) - /** * Called just before a build is started. * @@ -874,15 +823,10 @@ class GradleBuildService : /** * Called when a build is successful. * - * @param buildId The build that succeeded. [tasks] is the server's own list and not the one - * [prepareBuild] was given, so this is the only thing that names the build. * @param tasks The tasks that were run. * @see IToolingApiClient.onBuildSuccessful */ - fun onBuildSuccessful( - buildId: BuildId, - tasks: List, - ) + fun onBuildSuccessful(tasks: List) /** * Called when a progress event is received from the Tooling API server. @@ -894,17 +838,17 @@ class GradleBuildService : /** * Called when a build fails. * - * A build the user cancelled arrives here too; compare [buildId] with the one - * [onBuildCancelRequested] named to tell the two apart. + * A build the user cancelled arrives here too, and [failure] is what tells the two apart. + * It comes from the server, which classifies the throwable Gradle raised -- the only place + * the answer is known rather than inferred (ADFA-5542). * - * @param buildId The build that failed. [tasks] is the server's own list and not the one - * [prepareBuild] was given, so this is the only thing that names the build. * @param tasks The tasks that were run. + * @param failure Why the build failed, or null if the server did not say. * @see IToolingApiClient.onBuildFailed */ fun onBuildFailed( - buildId: BuildId, tasks: List, + failure: TaskExecutionResult.Failure?, ) /** diff --git a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt index 466bdf8b39..c2dc48a8b8 100644 --- a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt +++ b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt @@ -18,9 +18,10 @@ package com.itsaky.androidide.handlers import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage import com.itsaky.androidide.tooling.api.messages.BuildId -import com.itsaky.androidide.tooling.api.messages.BuildRunType import com.itsaky.androidide.tooling.api.messages.result.BuildInfo +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import com.itsaky.androidide.tooling.events.ProgressEvent import com.itsaky.androidide.tooling.events.internal.DefaultOperationDescriptor import com.itsaky.androidide.tooling.events.internal.DefaultProgressEvent @@ -37,17 +38,16 @@ import org.robolectric.RobolectricTestRunner /** * What the metrics charts annotate, and which build each annotation belongs to. * - * Two decisions, both asserted against the predicate that makes them rather than through the + * Two decisions, both asserted against the function that makes them rather than through the * callback that acts on it -- those need a live activity before they get this far. Which progress * events are marked at all (ADFA-5486), and whether a build that failed was really the user - * stopping it (ADFA-5542). + * stopping it (ADFA-5542). The second is now a reading of what the server said rather than a + * conclusion drawn on this side, so what is worth pinning is which answers are *not* a cancel. */ @RunWith(RobolectricTestRunner::class) class EditorBuildEventListenerAnnotationTest { private val listener = EditorBuildEventListener() - private fun buildId(id: Long) = BuildId(buildSessionId = "session", buildId = id, runType = BuildRunType.TaskRun) - private fun taskDescriptor() = TaskOperationDescriptor( dependencies = emptySet(), @@ -80,45 +80,48 @@ class EditorBuildEventListenerAnnotationTest { ) @Test - fun `a cancel that lands before its build is prepared still marks that build cancelled`() { - // The interleaving ADFA-5542 is about, and the one the main thread can really produce: - // onBuildCancelRequested is raised on the UI thread and runs inline, prepareBuild is - // raised from the build's own thread and is posted, so the cancel can overtake it. When - // the listener held a bare flag, prepareBuild cleared it and the build the user stopped - // was reported back to them as a failure. - listener.onBuildCancelRequested(buildId(7)) - listener.prepareBuild(BuildInfo(buildId(7), listOf(":app:assembleDebug"))) - - assertThat(listener.outcomeKind(buildId(7))) + fun `the server saying a build was cancelled is what marks it cancelled`() { + // The listener used to answer this from a flag it set when the cancel was asked for, which + // meant deciding from the order two main-thread runnables happened to run in -- and a + // cancel that overtook prepareBuild was cleared by it. Nothing here depends on order any + // more: the server classifies the throwable Gradle raised and this reads the answer. + assertThat(listener.outcomeKind(TaskExecutionResult.Failure.BUILD_CANCELLED)) .isEqualTo(MetricsAnnotationStore.Kind.BUILD_CANCELLED) } @Test - fun `a cancel is not inherited by the next build`() { - // The other half of keying to a build rather than to a moment. This listener outlives any - // one activity, so a cancel whose outcome never arrived stays held -- and must not relabel - // the next build's genuine failure. - listener.onBuildCancelRequested(buildId(7)) - - assertThat(listener.outcomeKind(buildId(8))) - .isEqualTo(MetricsAnnotationStore.Kind.BUILD_FAILED) + fun `every other reason the server gives is a failure`() { + // The substance of the mapping, and the half worth pinning: a connection that dropped or a + // Gradle version that is not supported is not the user stopping anything, and reporting it + // as one would tell them their own action broke a build they never touched. + val notCancels = + TaskExecutionResult.Failure.entries.filterNot { it == TaskExecutionResult.Failure.BUILD_CANCELLED } + notCancels.forEach { failure -> + assertWithMessage(failure.name) + .that(listener.outcomeKind(failure)) + .isEqualTo(MetricsAnnotationStore.Kind.BUILD_FAILED) + } } @Test - fun `a build nobody stopped is a failure`() { - assertThat(listener.outcomeKind(buildId(7))) - .isEqualTo(MetricsAnnotationStore.Kind.BUILD_FAILED) + fun `a failure the server did not classify is a failure`() { + // Null reaches here from any path that reports a build result without a reason. Treating + // an absent answer as a cancel would put the user's name on something they did not do. + assertThat(listener.outcomeKind(null)).isEqualTo(MetricsAnnotationStore.Kind.BUILD_FAILED) } @Test - fun `a cancel naming no build leaves the one already held alone`() { - // cancelCurrentBuild passes null when nothing is running. Taking that as "forget the - // cancel" would lose the attribution for a build still finishing. - listener.onBuildCancelRequested(buildId(7)) - listener.onBuildCancelRequested(null) + fun `preparing a build clears a stale pairing, even with no activity attached`() { + listener.annotatedBuild = true - assertThat(listener.outcomeKind(buildId(7))) - .isEqualTo(MetricsAnnotationStore.Kind.BUILD_CANCELLED) + // No activity is attached here, so prepareBuild returns early -- which is the point. The + // flag means "a start marker was drawn for the build now running", and this listener + // outlives any one activity, so a build whose outcome arrived without one would otherwise + // leave it set for the next build to inherit and draw a finish for a build that never + // started. + listener.prepareBuild(BuildInfo(BuildId.Unknown, listOf(":app:assembleDebug"))) + + assertThat(listener.annotatedBuild).isFalse() } @Test diff --git a/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt index a8d8ca2f33..5eac7a65cb 100644 --- a/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt @@ -19,7 +19,7 @@ package com.itsaky.androidide.services.builder import com.google.common.truth.Truth.assertThat import com.itsaky.androidide.services.builder.GradleBuildService.EventListener -import com.itsaky.androidide.tooling.api.messages.BuildId +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -28,12 +28,16 @@ import java.lang.reflect.Proxy /** * Pins that the build service's listener wrapper forwards every callback it is given. * - * It did not. `onBuildCancelRequested` was declared with a `= Unit` default so that only listeners - * that cared had to implement it; the wrapper then inherited that no-op rather than passing the - * call on, so the cancel never reached the real listener and a build the user had stopped went on - * being annotated as a failure -- which is what BUILD_CANCELLED exists to prevent. The feature was - * unreachable, and the store-level test for it passed the whole time, because it called the store - * directly and nothing exercised the path to it. + * It did not. A now-removed `onBuildCancelRequested` was declared with a `= Unit` default so that + * only listeners that cared had to implement it; the wrapper then inherited that no-op rather than + * passing the call on, so the cancel never reached the real listener and a build the user had + * stopped went on being annotated as a failure -- which is what BUILD_CANCELLED exists to prevent. + * The feature was unreachable, and the store-level test for it passed the whole time, because it + * called the store directly and nothing exercised the path to it. + * + * That callback is gone: the server classifies the throwable Gradle raised and says so on the + * BuildResult, so nothing on this side has to be told separately (ADFA-5542). The invariant it + * left behind outlives it and still guards every remaining member. */ @RunWith(RobolectricTestRunner::class) class GradleBuildServiceListenerWrapperTest { @@ -75,30 +79,21 @@ class GradleBuildServiceListenerWrapperTest { } @Test - fun `a cancel request reaches the listener, naming its build`() { - val recorder = Recorder() - val wrapped = GradleBuildService.wrap(recorder.listener)!! - - wrapped.onBuildCancelRequested(BuildId.Unknown) - - // The one this went wrong on, kept as its own case so the reason is legible in a report. - assertThat(recorder.calls).containsExactly("onBuildCancelRequested") - - // The id is the whole of what makes a cancel attributable (ADFA-5542). A wrapper that - // forwarded the call and dropped the argument would be the same defect one layer in, and - // no signature would complain about it. - assertThat(recorder.lastArgs).containsExactly(BuildId.Unknown) - } - - @Test - fun `an outcome reaches the listener with the build it belongs to`() { + fun `a failure reaches the listener with the server's reason for it`() { val recorder = Recorder() val wrapped = GradleBuildService.wrap(recorder.listener)!! - wrapped.onBuildFailed(BuildId.Unknown, listOf(":app:assembleDebug")) + wrapped.onBuildFailed(listOf(":app:assembleDebug"), TaskExecutionResult.Failure.BUILD_CANCELLED) assertThat(recorder.calls).containsExactly("onBuildFailed") - assertThat(recorder.lastArgs).containsExactly(BuildId.Unknown, listOf(":app:assembleDebug")).inOrder() + + // The reason is the whole of what tells a cancel from a failure (ADFA-5542), and it is the + // server's answer rather than one this side worked out. A wrapper that forwarded the call + // and dropped the argument would be the earlier defect one layer in, with no signature to + // complain about it. + assertThat(recorder.lastArgs) + .containsExactly(listOf(":app:assembleDebug"), TaskExecutionResult.Failure.BUILD_CANCELLED) + .inOrder() } @Test 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 9a05bacaac..09e29b2ecf 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 @@ -143,14 +143,18 @@ internal class ToolingApiServerImpl : IToolingApiServer { return@runBuild doInitialize(params, start) } catch (err: Throwable) { log.error("Failed to initialize project", err) + // One classification, used twice. Told only through the return value, the client + // had no way to tell a sync the user stopped from one that broke (ADFA-5542). + val failure = getTaskFailureType(err) notifyBuildFailure( BuildResult( tasks = emptyList(), buildId = params.buildId, durationMs = System.currentTimeMillis() - start, + failure = failure, ), ) - return@runBuild InitializeResult.Failure(getTaskFailureType(err)) + return@runBuild InitializeResult.Failure(failure) } } } @@ -317,15 +321,17 @@ internal class ToolingApiServerImpl : IToolingApiServer { return@runBuild TaskExecutionResult.SUCCESS } catch (error: Throwable) { log.error("Failed to run tasks: {}", message.tasks, error) + val failure = getTaskFailureType(error) notifyBuildFailure( result = BuildResult( tasks = message.tasks, buildId = message.buildId, durationMs = System.currentTimeMillis() - start, + failure = failure, ), ) - return@runBuild TaskExecutionResult(false, getTaskFailureType(error)) + return@runBuild TaskExecutionResult(false, failure) } } } 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 21a346df7e..20fa2844a1 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,10 @@ 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.InitializeProjectParams +import com.itsaky.androidide.tooling.api.messages.result.BuildResult 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 @@ -10,8 +13,10 @@ import com.itsaky.androidide.tooling.impl.sync.RootModelBuilder import io.mockk.every import io.mockk.mockk import io.mockk.mockkObject +import io.mockk.slot import io.mockk.spyk import io.mockk.verify +import org.gradle.tooling.BuildCancelledException import org.gradle.tooling.GradleConnector import org.gradle.tooling.ProjectConnection import org.junit.Test @@ -25,18 +30,21 @@ import java.util.concurrent.TimeUnit */ @RunWith(JUnit4::class) class ToolingApiServerImplTest { - private fun testInitParams( directory: String = "/does/not/exist", forceSync: Boolean = false, ) = InitializeProjectParams( - directory = directory, needsGradleSync = forceSync + // Required since ADFA-2784 added it, and never supplied here: this file has not compiled + // on stage since, and no workflow runs :subprojects: tests, so nothing said so. + buildId = BuildId.Unknown, + directory = directory, + needsGradleSync = forceSync, ) private data class MockServer( val server: ToolingApiServerImpl, val connector: GradleConnector, - val connection: ProjectConnection + val connection: ProjectConnection, ) private fun mockkToolingServer(): MockServer { @@ -47,7 +55,10 @@ class ToolingApiServerImplTest { // ensure that we do not start actual Gradle build every { server.getOrConnectProject( - projectDir = any(), forceConnect = true, initParams = any(), gradleDist = any() + projectDir = any(), + forceConnect = true, + initParams = any(), + gradleDist = any(), ) } returns (connector to connection) @@ -56,12 +67,12 @@ class ToolingApiServerImplTest { @Test fun `GIVEN any initialization params WHEN project init fails THEN report as failure`() { - mockkObject(RootModelBuilder) every { // Simulate a Gradle sync failure RootModelBuilder.build( - any(), any() + any(), + any(), ) } throws RuntimeException("intentional failure") @@ -82,8 +93,38 @@ class ToolingApiServerImplTest { } @Test - fun `GIVEN force sync not requested WHEN sync files are unreadable THEN sync anyway`() { + fun `GIVEN a build the user stopped WHEN it fails THEN the client is told it was a cancel`() { + mockkObject(RootModelBuilder) + every { + // Gradle raises this, and only this, for a build that was cancelled. + RootModelBuilder.build(any(), any()) + } throws BuildCancelledException("stopped by the user") + val (server) = mockkToolingServer() + + every { + server.validateProjectDirectory(any()) + } returns null + + val client = mockk(relaxed = true) + server.connect(client) + + val result = server.initialize(testInitParams()).get(5, TimeUnit.SECONDS) + assertThat((result as InitializeResult.Failure).failure) + .isEqualTo(TaskExecutionResult.Failure.BUILD_CANCELLED) + + // The same verdict has to reach the client, not only the caller of initialize. It did not, + // and the editor was left reconstructing "was that a cancel?" from the order its own + // callbacks happened to arrive in -- which it got wrong, annotating a build the user had + // stopped as a failure (ADFA-5542). This is the one place the answer is known rather than + // inferred, and it is one classification of one throwable, used for both. + val reported = slot() + verify { client.onBuildFailed(capture(reported)) } + assertThat(reported.captured.failure).isEqualTo(TaskExecutionResult.Failure.BUILD_CANCELLED) + } + + @Test + fun `GIVEN force sync not requested WHEN sync files are unreadable THEN sync anyway`() { val initParams = testInitParams(forceSync = false) val cacheFile = ProjectSyncHelper.cacheFileForProject(File(initParams.directory)) @@ -91,7 +132,8 @@ class ToolingApiServerImplTest { every { // simulate a successful cache write RootModelBuilder.build( - any(), any() + any(), + any(), ) } returns cacheFile diff --git a/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt b/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt index 46fb9babba..3732a072d5 100644 --- a/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt +++ b/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt @@ -28,4 +28,14 @@ data class BuildResult( val buildId: BuildId, val tasks: List, val durationMs: Long, + /** + * Why the build failed, or `null` if it succeeded. + * + * The server is the only party that can answer this: Gradle raises a + * `BuildCancelledException` for a build the user stopped, and the same throwable that decides + * the [TaskExecutionResult] decides this. Without it a client had to reconstruct "was that a + * cancel?" from the order its own callbacks happened to arrive in, and got it wrong + * (ADFA-5542). + */ + val failure: TaskExecutionResult.Failure? = null, ) From 29e07894dace442442b7279af24dc5e47b215c44 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 19:03:41 -0700 Subject: [PATCH 3/5] ADFA-5542: report a cancelled build as cancelled everywhere, not just on the chart Two findings from the second review. The sibling sweep was not done. The chart marker was fixed and the three reports beside it were left saying "failed", so a user who pressed Stop got a red "Build failed" bar, a "Build failed" notification in the shade, and an isSuccess=false result carrying failure text posted to every plugin listener -- their own deliberate action read back to them as an error in every place but one. The argument the PR gave for the marker applies verbatim to all four, and `failure` was already in hand at each. The notification, the bar and the message now follow it. The plugin API cannot express a cancel at all -- IdeServices.onBuildFailed takes an error string and nothing else -- so the message is the whole of what a plugin can be told, and that is now said in the code. The regression test drove initialize(), not executeTasks(), which is the path the ticket's defect actually travels: both sites got the same two-line edit, so deleting one left the suite green. Rather than stand up a live ProjectConnection to test the second site, the two are now one call. notifyBuildFailure classifies the throwable, notifies the client and returns the answer to its caller, so a site cannot report a failure without saying which, or tell the client one thing and its caller another. There is one place left to get this wrong, and the existing test covers it -- dropping the field still fails it and nothing else. The message is extracted as failureMessage for the same reason outcomeKind is: onBuildFailed returns early without an activity, so anything decided inside it cannot be reached from a test. The cancelled text is passed in rather than resolved, which is what makes it assertable. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../handlers/EditorBuildEventListener.kt | 36 +++++++++++-- .../services/builder/GradleBuildService.kt | 10 +++- .../EditorBuildEventListenerAnnotationTest.kt | 22 ++++++++ .../tooling/impl/ToolingApiServerImpl.kt | 54 +++++++++++-------- .../tooling/impl/ToolingApiServerImplTest.kt | 9 +++- 5 files changed, 102 insertions(+), 29 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt index 4f4196f731..4abae99862 100644 --- a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt +++ b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt @@ -34,6 +34,7 @@ import com.itsaky.androidide.tooling.events.task.TaskFinishEvent import com.itsaky.androidide.tooling.events.task.TaskStartEvent import com.itsaky.androidide.utils.MetricsAnnotationStore import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.flashInfo import com.itsaky.androidide.utils.flashSuccess import com.itsaky.androidide.viewmodel.BuildOutputViewModel import org.slf4j.LoggerFactory @@ -135,6 +136,24 @@ class EditorBuildEventListener : GradleBuildService.EventListener { } } + /** + * What a failed build is reported as, to the plugins and in the result the editor posts. + * + * [cancelledText] is passed in rather than resolved here so this can be asserted without an + * activity, for the same reason [outcomeKind] is separate: [onBuildFailed] returns early + * without one, so anything decided inside it is unreachable from a test. + */ + @VisibleForTesting + internal fun failureMessage( + failure: TaskExecutionResult.Failure?, + cancelledText: String, + ): String = + when { + failure == TaskExecutionResult.Failure.BUILD_CANCELLED -> cancelledText + lastStatusLine.contains("BUILD FAILED") -> lastStatusLine + else -> "Build failed. Check build output for details." + } + /** * Which marker a failed build gets: the user's own cancel, or a real failure (ADFA-5542). * @@ -226,6 +245,8 @@ class EditorBuildEventListener : GradleBuildService.EventListener { ) { val act = checkActivity("onBuildFailed") ?: return + val cancelled = failure == TaskExecutionResult.Failure.BUILD_CANCELLED + if (annotatedBuild) { // A build the user stopped arrives through this same callback. Marking it as a failure // would report their own deliberate action back to them in the error colour. @@ -236,11 +257,20 @@ class EditorBuildEventListener : GradleBuildService.EventListener { analyzeCurrentFile() GeneralPreferences.isFirstBuild = false act.editorViewModel.isBuildInProgress = false - act.flashError(R.string.build_status_failed) + // Everything this method says, not only the chart marker. The annotation was fixed first + // and the three reports beside it were not, so a user who pressed Stop still got a red + // "Build failed" bar, a "Build failed" notification and an isSuccess=false result -- their + // own action read back to them as an error in every place but one. + if (cancelled) { + act.flashInfo(R.string.info_build_cancelled) + } else { + act.flashError(R.string.build_status_failed) + } - val message = - if (lastStatusLine.contains("BUILD FAILED")) lastStatusLine else "Build failed. Check build output for details." + val message = failureMessage(failure, act.getString(R.string.info_build_cancelled)) + // The plugin API has no way to say "cancelled" -- IdeServices.onBuildFailed takes an error + // string and nothing else -- so the message is the whole of what a plugin can be told. pluginBuildService?.notifyBuildFailed(message) act.notifyBuildResult(BuildResult(isSuccess = false, message = message, launchResult = null)) diff --git a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt index a2809db546..dcc0860a2f 100644 --- a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt +++ b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt @@ -460,7 +460,15 @@ class GradleBuildService : } override fun onBuildFailed(result: BuildResult) { - updateNotification(getString(R.string.build_status_failed), false) + // The notification too, not only what reaches the listener: a build the user stopped left + // "Build failed" in the shade whatever the chart said (ADFA-5542). + val status = + if (result.failure == TaskExecutionResult.Failure.BUILD_CANCELLED) { + R.string.info_build_cancelled + } else { + R.string.build_status_failed + } + updateNotification(getString(status), false) dispatchBuildResult(result, false) eventListener?.onBuildFailed(result.tasks, result.failure) diff --git a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt index c2dc48a8b8..effb235731 100644 --- a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt +++ b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt @@ -110,6 +110,23 @@ class EditorBuildEventListenerAnnotationTest { assertThat(listener.outcomeKind(null)).isEqualTo(MetricsAnnotationStore.Kind.BUILD_FAILED) } + @Test + fun `a build the user stopped is not reported as an error anywhere`() { + // The annotation was fixed first and the reports beside it were not, so a user who pressed + // Stop still got a red "Build failed" bar, a "Build failed" notification and an + // isSuccess=false result carrying failure text -- their own action read back to them as an + // error in every place but the chart. + assertThat(listener.failureMessage(TaskExecutionResult.Failure.BUILD_CANCELLED, CANCELLED_TEXT)) + .isEqualTo(CANCELLED_TEXT) + } + + @Test + fun `a build that really failed still says so`() { + assertThat(listener.failureMessage(TaskExecutionResult.Failure.BUILD_FAILED, CANCELLED_TEXT)) + .isNotEqualTo(CANCELLED_TEXT) + assertThat(listener.failureMessage(null, CANCELLED_TEXT)).isNotEqualTo(CANCELLED_TEXT) + } + @Test fun `preparing a build clears a stale pairing, even with no activity attached`() { listener.annotatedBuild = true @@ -140,4 +157,9 @@ class EditorBuildEventListenerAnnotationTest { // that matter under configuration noise. assertThat(listener.isAnnotated(plainEvent())).isFalse() } + + private companion object { + /** Stands in for the string the activity would resolve, which a test has no activity for. */ + const val CANCELLED_TEXT = "Build was cancelled by the user." + } } 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 09e29b2ecf..f76315071f 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 @@ -19,6 +19,7 @@ package com.itsaky.androidide.tooling.impl import com.itsaky.androidide.tooling.api.IToolingApiClient import com.itsaky.androidide.tooling.api.IToolingApiServer +import com.itsaky.androidide.tooling.api.messages.BuildId import com.itsaky.androidide.tooling.api.messages.ClientGradleBuildConfig import com.itsaky.androidide.tooling.api.messages.GradleDistributionParams import com.itsaky.androidide.tooling.api.messages.GradleDistributionType @@ -143,18 +144,9 @@ internal class ToolingApiServerImpl : IToolingApiServer { return@runBuild doInitialize(params, start) } catch (err: Throwable) { log.error("Failed to initialize project", err) - // One classification, used twice. Told only through the return value, the client - // had no way to tell a sync the user stopped from one that broke (ADFA-5542). - val failure = getTaskFailureType(err) - notifyBuildFailure( - BuildResult( - tasks = emptyList(), - buildId = params.buildId, - durationMs = System.currentTimeMillis() - start, - failure = failure, - ), + return@runBuild InitializeResult.Failure( + notifyBuildFailure(params.buildId, emptyList(), start, err), ) - return@runBuild InitializeResult.Failure(failure) } } } @@ -321,17 +313,10 @@ internal class ToolingApiServerImpl : IToolingApiServer { return@runBuild TaskExecutionResult.SUCCESS } catch (error: Throwable) { log.error("Failed to run tasks: {}", message.tasks, error) - val failure = getTaskFailureType(error) - notifyBuildFailure( - result = - BuildResult( - tasks = message.tasks, - buildId = message.buildId, - durationMs = System.currentTimeMillis() - start, - failure = failure, - ), + return@runBuild TaskExecutionResult( + false, + notifyBuildFailure(message.buildId, message.tasks, start, error), ) - return@runBuild TaskExecutionResult(false, failure) } } } @@ -363,8 +348,31 @@ internal class ToolingApiServerImpl : IToolingApiServer { } } - private fun notifyBuildFailure(result: BuildResult) { - client?.onBuildFailed(result) + /** + * Tells the client a build failed, and answers with why. + * + * Both in one call on purpose. The classification and the notification used to be written + * separately at each failure site, which is how the notified [BuildResult] came to carry + * everything except the answer while the caller of the request got it (ADFA-5542). A site + * cannot now report a failure without saying which, or say one thing to the client and another + * to its caller. + */ + private fun notifyBuildFailure( + buildId: BuildId, + tasks: List, + startedAtMillis: Long, + error: Throwable, + ): Failure { + val failure = getTaskFailureType(error) + client?.onBuildFailed( + BuildResult( + buildId = buildId, + tasks = tasks, + durationMs = System.currentTimeMillis() - startedAtMillis, + failure = failure, + ), + ) + return failure } private fun notifyBuildSuccess(result: BuildResult) { 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 20fa2844a1..8c399e4cc7 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 @@ -116,8 +116,13 @@ class ToolingApiServerImplTest { // The same verdict has to reach the client, not only the caller of initialize. It did not, // and the editor was left reconstructing "was that a cancel?" from the order its own // callbacks happened to arrive in -- which it got wrong, annotating a build the user had - // stopped as a failure (ADFA-5542). This is the one place the answer is known rather than - // inferred, and it is one classification of one throwable, used for both. + // stopped as a failure (ADFA-5542). + // + // This drives the sync path. The task-run path is the one the ticket is really about, and + // standing it up needs a live ProjectConnection; instead of testing the two separately, + // notifyBuildFailure now classifies and notifies in one call and hands the answer back, so + // neither site can report a failure without saying which, or tell the client one thing and + // its caller another. There is one place left to get this wrong and this covers it. val reported = slot() verify { client.onBuildFailed(capture(reported)) } assertThat(reported.captured.failure).isEqualTo(TaskExecutionResult.Failure.BUILD_CANCELLED) From a18fdbdce912c138ef6e712442fbc0674c0e459f Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 11:16:57 -0700 Subject: [PATCH 4/5] ADFA-5542: finish the sweep, and stop leaking the cancellation token The sibling sweep missed the path its own test drives. A sync the user stops arrives at ProjectHandlerActivity.postProjectInit as a failure carrying BUILD_CANCELLED, and the `when` there has no arm for it, so pressing Stop during a sync produced an indefinite red "Project initialization failed". The server-side regression test added last round exercises exactly this path, which makes "not reported as an error anywhere" false where it was most confidently claimed. The cancellation token outlived every build it belonged to. executeTasks cleared it on success and not on failure; the sync path never cleared it on any outcome at all, only shutdown() and an actual Stop did. So after any sync, and after any failed build, a token for a finished source sat in the server: the next Stop cancelled that dead source and answered wasEnqueued = true with nothing running -- and once a real build had started, left it running while telling the user it was being stopped. initialize(), which cancels first whenever a token is set, paid for a build that had ended long before. Both paths clear it in a finally now, and two tests fail without it. Removing onBuildCancelRequested took away the only request-time signal and nothing replaced the half that mattered. A Stop the server refuses -- NO_RUNNING_BUILD, or Gradle declining -- reached a log line at one call site and nothing whatsoever at the other: EditorPanelDockableContent threw the result away entirely. Both go through one reporter now, which says so. It also stops dereferencing failureReason with !!, which would have crashed on a refusal that carried no reason. Three more places still called a cancel a failure. BuildViewModel threw RuntimeException("Task execution failed: BUILD_CANCELLED") -- a cancel does not arrive as a CancellationException, so its catch could not tell, and the enum name was shown to the user as an error. The status line under the build output kept Gradle's own "BUILD FAILED", which onOutput copies there, contradicting the bar that had just said the build was stopped. And telemetry recorded success=false with no reason at all, so a deliberate cancel and a broken build were indistinguishable and every build-success rate counted the one the IDE makes easiest to press. The predicate is defined once now rather than derived three times in one callback, which is the duplicated-comparison finding from the previous round, reintroduced. The two KDocs contradicted each other about null. BuildResult.failure said null meant success; EventListener.onBuildFailed said it meant the server did not say. From this server it can be neither on a failure -- notifyBuildFailure classifies and returns non-null, and both catch paths go through it -- so the nullability exists for a server that does not classify. Both say that now, and the tests that cover null say they cover a defensive default rather than a reachable path. The test named "not reported as an error anywhere" asserted one of the four regressions it listed, so deleting the notification branch left it green. It is renamed to what it checks, and the notification and the telemetry reason now have tests of their own. Still unpinned, and said so in the test: the bar itself, and the cancelled-sync branch. Both need a live activity, which is what onBuildFailed returns early without. Not fixed, deliberately: info_build_cancelled exists in 3 of 14 locales and build_status_failed in 13, so ten locales trade a translated "Build failed" for an English "Build was cancelled by the user." I am not inventing translations. Every recently added string here is 3/14, so English fallback is this project's standing state rather than something this ticket introduced -- but the trade is real, and correct-in-English beats translated-and-wrong for a message that accuses the user of a failure they did not cause. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../build/AbstractCancellableRunAction.kt | 24 +----- .../editor/ProjectHandlerActivity.kt | 14 ++++ .../analytics/gradle/BuildCompletedMetric.kt | 6 ++ .../floating/EditorPanelDockableContent.kt | 5 +- .../handlers/EditorBuildEventListener.kt | 24 +++++- .../services/builder/GradleBuildService.kt | 29 +++++-- .../androidide/utils/BuildCancellation.kt | 63 +++++++++++++++ .../androidide/viewmodel/BuildViewModel.kt | 13 +++- .../analytics/BuildCompletedMetricTest.kt | 78 +++++++++++++++++++ .../EditorBuildEventListenerAnnotationTest.kt | 14 ++-- ...radleBuildServiceNotificationStatusTest.kt | 62 +++++++++++++++ resources/src/main/res/values/strings.xml | 1 + .../tooling/impl/ToolingApiServerImpl.kt | 39 +++++++--- .../tooling/impl/ToolingApiServerImplTest.kt | 39 ++++++++++ .../api/messages/result/BuildResult.kt | 8 +- 15 files changed, 366 insertions(+), 53 deletions(-) create mode 100644 app/src/main/java/com/itsaky/androidide/utils/BuildCancellation.kt create mode 100644 app/src/test/java/com/itsaky/androidide/analytics/BuildCompletedMetricTest.kt create mode 100644 app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceNotificationStatusTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/actions/build/AbstractCancellableRunAction.kt b/app/src/main/java/com/itsaky/androidide/actions/build/AbstractCancellableRunAction.kt index aea4c2b0e0..445a19d48f 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/build/AbstractCancellableRunAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/build/AbstractCancellableRunAction.kt @@ -12,6 +12,7 @@ import com.itsaky.androidide.lookup.Lookup import com.itsaky.androidide.projects.builder.BuildService import com.itsaky.androidide.resources.R import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.requestBuildCancellation import org.slf4j.Logger import org.slf4j.LoggerFactory @@ -77,34 +78,13 @@ abstract class AbstractCancellableRunAction( protected abstract fun doExec(data: ActionData): Any protected fun cancelBuild(): Boolean { - log.info("Sending build cancellation request...") val builder = Lookup.getDefault().lookup(BuildService.KEY_BUILD_SERVICE) if (builder?.isToolingServerStarted() != true) { flashError(com.itsaky.androidide.projects.R.string.msg_tooling_server_unavailable) return false } - builder.cancelCurrentBuild().whenComplete { - result, - error, - -> - if (error != null) { - log.error("Failed to send build cancellation request", error) - return@whenComplete - } - - if (!result.wasEnqueued) { - log.warn( - "Unable to enqueue cancellation request reason={} reason.message={}", - result.failureReason, - result.failureReason!!.message, - ) - return@whenComplete - } - - log.info("Build cancellation request was successfully enqueued...") - } - + requestBuildCancellation(builder) return true } diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt index afc6707577..716a225cf6 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt @@ -80,6 +80,7 @@ import com.itsaky.androidide.tooling.api.messages.BuildRunType 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 +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.BUILD_CANCELLED import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.CACHE_READ_ERROR import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.PROJECT_DIRECTORY_INACCESSIBLE import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.PROJECT_NOT_DIRECTORY @@ -95,6 +96,7 @@ import com.itsaky.androidide.utils.DialogUtils.showRestartPrompt import com.itsaky.androidide.utils.RecursiveFileSearcher import com.itsaky.androidide.utils.dpToPx import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.flashInfo import com.itsaky.androidide.utils.flashSuccess import com.itsaky.androidide.utils.flashbarBuilder import com.itsaky.androidide.utils.onLongPress @@ -797,6 +799,18 @@ abstract class ProjectHandlerActivity : BaseEditorActivity() { manager.projectDir.name } + // A sync the user stopped is not a failure, and arrives here through the same callback + // as one. ADFA-5542 fixed that for builds and missed this path, which is the one a + // cancelled *sync* takes: the user pressed Stop and got an indefinite red "Project + // initialization failed" for doing so. + if (failure == BUILD_CANCELLED) { + val cancelled = getString(string.info_build_cancelled) + setStatus(cancelled) + flashInfo(cancelled) + editorViewModel.isInitializing = false + return + } + val initFailed = if (projectName.isNotEmpty()) { getString(string.msg_project_initialization_failed_with_name, projectName) diff --git a/app/src/main/java/com/itsaky/androidide/analytics/gradle/BuildCompletedMetric.kt b/app/src/main/java/com/itsaky/androidide/analytics/gradle/BuildCompletedMetric.kt index c095b08320..f216a7e0ac 100644 --- a/app/src/main/java/com/itsaky/androidide/analytics/gradle/BuildCompletedMetric.kt +++ b/app/src/main/java/com/itsaky/androidide/analytics/gradle/BuildCompletedMetric.kt @@ -20,5 +20,11 @@ class BuildCompletedMetric( putString("build_type", buildType) putBoolean("success", isSuccess) putLong("duration_ms", buildResult.durationMs) + // Why it was not successful, which the metric used to drop. A build the user stopped + // and a build that broke both arrive as success=false, so without this the two are + // indistinguishable and every build-success rate counts deliberate cancels as + // failures. isSuccess keeps its meaning -- a cancelled build did not succeed -- and a + // consumer that wants the rate excluding cancels can now compute it. + buildResult.failure?.let { putString("failure_reason", it.name) } } } diff --git a/app/src/main/java/com/itsaky/androidide/editor/floating/EditorPanelDockableContent.kt b/app/src/main/java/com/itsaky/androidide/editor/floating/EditorPanelDockableContent.kt index cee0b2ad74..918a2ef2f2 100644 --- a/app/src/main/java/com/itsaky/androidide/editor/floating/EditorPanelDockableContent.kt +++ b/app/src/main/java/com/itsaky/androidide/editor/floating/EditorPanelDockableContent.kt @@ -19,6 +19,7 @@ import com.itsaky.androidide.models.Position import com.itsaky.androidide.models.Range import com.itsaky.androidide.projects.builder.BuildService import com.itsaky.androidide.ui.CodeEditorView +import com.itsaky.androidide.utils.requestBuildCancellation import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -114,7 +115,9 @@ class EditorPanelDockableContent( private fun cancelBuild() { val builder = Lookup.getDefault().lookup(BuildService.KEY_BUILD_SERVICE) if (builder?.isToolingServerStarted() == true) { - builder.cancelCurrentBuild() + // Through the shared reporter, because this copy threw the result away entirely: a + // Stop the server refused here said nothing at all, not even to the log. + requestBuildCancellation(builder) } } diff --git a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt index 4abae99862..685b859113 100644 --- a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt +++ b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt @@ -136,6 +136,16 @@ class EditorBuildEventListener : GradleBuildService.EventListener { } } + /** + * Whether [failure] is the user's own Stop rather than something going wrong. + * + * One definition, because this callback used to ask the same question three times -- once in + * [failureMessage], once in [outcomeKind] and once inline -- which is how the chart and the + * messages beside it came to disagree in the first place. + */ + @VisibleForTesting + internal fun isCancelled(failure: TaskExecutionResult.Failure?): Boolean = failure == TaskExecutionResult.Failure.BUILD_CANCELLED + /** * What a failed build is reported as, to the plugins and in the result the editor posts. * @@ -149,7 +159,7 @@ class EditorBuildEventListener : GradleBuildService.EventListener { cancelledText: String, ): String = when { - failure == TaskExecutionResult.Failure.BUILD_CANCELLED -> cancelledText + isCancelled(failure) -> cancelledText lastStatusLine.contains("BUILD FAILED") -> lastStatusLine else -> "Build failed. Check build output for details." } @@ -168,7 +178,7 @@ class EditorBuildEventListener : GradleBuildService.EventListener { */ @VisibleForTesting internal fun outcomeKind(failure: TaskExecutionResult.Failure?): MetricsAnnotationStore.Kind = - if (failure == TaskExecutionResult.Failure.BUILD_CANCELLED) { + if (isCancelled(failure)) { MetricsAnnotationStore.Kind.BUILD_CANCELLED } else { MetricsAnnotationStore.Kind.BUILD_FAILED @@ -245,7 +255,7 @@ class EditorBuildEventListener : GradleBuildService.EventListener { ) { val act = checkActivity("onBuildFailed") ?: return - val cancelled = failure == TaskExecutionResult.Failure.BUILD_CANCELLED + val cancelled = isCancelled(failure) if (annotatedBuild) { // A build the user stopped arrives through this same callback. Marking it as a failure @@ -261,13 +271,19 @@ class EditorBuildEventListener : GradleBuildService.EventListener { // and the three reports beside it were not, so a user who pressed Stop still got a red // "Build failed" bar, a "Build failed" notification and an isSuccess=false result -- their // own action read back to them as an error in every place but one. + val cancelledText = act.getString(R.string.info_build_cancelled) if (cancelled) { act.flashInfo(R.string.info_build_cancelled) + // The status line under the output too. Gradle prints "BUILD FAILED" for a cancelled + // build like any other, and [onOutput] copies that line into the label, so the label + // sat there contradicting the bar that had just said the build was stopped. This runs + // after onOutput, so it has the last word. + act.setStatus(cancelledText) } else { act.flashError(R.string.build_status_failed) } - val message = failureMessage(failure, act.getString(R.string.info_build_cancelled)) + val message = failureMessage(failure, cancelledText) // The plugin API has no way to say "cancelled" -- IdeServices.onBuildFailed takes an error // string and nothing else -- so the message is the whole of what a plugin can be told. diff --git a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt index dcc0860a2f..e5feb398ef 100644 --- a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt +++ b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt @@ -459,16 +459,26 @@ class GradleBuildService : eventListener?.onBuildSuccessful(result.tasks) } + /** + * What the notification in the shade says about a build that did not succeed. + * + * Extracted so it can be asserted: [onBuildFailed] needs a live service before it reaches this + * point, which is the same reason [EditorBuildEventListener] separates its own two decisions. + * Without a test here, deleting the cancelled arm left every test green while a build the user + * stopped went back to saying "Build failed" in the shade. + */ + @VisibleForTesting + internal fun notificationStatusFor(failure: TaskExecutionResult.Failure?): Int = + if (failure == TaskExecutionResult.Failure.BUILD_CANCELLED) { + R.string.info_build_cancelled + } else { + R.string.build_status_failed + } + override fun onBuildFailed(result: BuildResult) { // The notification too, not only what reaches the listener: a build the user stopped left // "Build failed" in the shade whatever the chart said (ADFA-5542). - val status = - if (result.failure == TaskExecutionResult.Failure.BUILD_CANCELLED) { - R.string.info_build_cancelled - } else { - R.string.build_status_failed - } - updateNotification(getString(status), false) + updateNotification(getString(notificationStatusFor(result.failure)), false) dispatchBuildResult(result, false) eventListener?.onBuildFailed(result.tasks, result.failure) @@ -851,7 +861,10 @@ class GradleBuildService : * the answer is known rather than inferred (ADFA-5542). * * @param tasks The tasks that were run. - * @param failure Why the build failed, or null if the server did not say. + * @param failure Why the build failed. Never null from this server, which classifies every + * failure before reporting it; nullable because the wire type allows a server that does + * not. A null is treated as an ordinary failure, which is the safe reading -- reporting + * a real failure as a cancel would hide it. * @see IToolingApiClient.onBuildFailed */ fun onBuildFailed( diff --git a/app/src/main/java/com/itsaky/androidide/utils/BuildCancellation.kt b/app/src/main/java/com/itsaky/androidide/utils/BuildCancellation.kt new file mode 100644 index 0000000000..af0b83cb2c --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/utils/BuildCancellation.kt @@ -0,0 +1,63 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import com.itsaky.androidide.projects.builder.BuildService +import com.itsaky.androidide.resources.R +import org.slf4j.LoggerFactory + +private val log = LoggerFactory.getLogger("BuildCancellation") + +/** + * Asks [service] to stop the running build, and says so when it will not. + * + * A Stop the server turns down -- there is no build to cancel, or Gradle refused the request -- + * reached a log line at one call site and nothing at all at the other, so the button appeared to + * do nothing and the build carried on. ADFA-5542 removed `EventListener.onBuildCancelRequested`, + * which was the only request-time signal, because it guessed the outcome rather than reporting + * one; nothing replaced the half of it that mattered. This is that half, in one place, because + * two call sites written separately are how they came to disagree. + * + * Success is deliberately silent: the build stopping is its own feedback, and + * [com.itsaky.androidide.handlers.EditorBuildEventListener.onBuildFailed] reports the outcome the + * server actually reached. + */ +fun requestBuildCancellation(service: BuildService) { + log.info("Sending build cancellation request...") + service.cancelCurrentBuild().whenComplete { result, error -> + if (error != null) { + log.error("Failed to send build cancellation request", error) + flashError(R.string.msg_build_cancel_failed) + return@whenComplete + } + + if (!result.wasEnqueued) { + // failureReason is nullable on the wire, so this reads it rather than asserting it. + // A refusal with no reason still has to reach the user. + log.warn( + "Unable to enqueue cancellation request reason={} reason.message={}", + result.failureReason, + result.failureReason?.message, + ) + flashError(R.string.msg_build_cancel_failed) + return@whenComplete + } + + log.info("Build cancellation request was successfully enqueued...") + } +} diff --git a/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt b/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt index 8c90924c2a..b5234746c1 100644 --- a/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt +++ b/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt @@ -13,6 +13,7 @@ import com.itsaky.androidide.projects.models.assembleTaskOutputListingFile import com.itsaky.androidide.tooling.api.messages.BuildRunType import com.itsaky.androidide.tooling.api.messages.GradleBuildParams import com.itsaky.androidide.tooling.api.messages.TaskExecutionMessage +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -110,7 +111,17 @@ class BuildViewModel : ViewModel() { }.await() if (result == null || !result.isSuccessful) { - throw RuntimeException("Task execution failed: ${result.failure}") + // A build the user stopped is not a failure, and it does not arrive as a + // CancellationException -- the catch below only recognises the coroutine kind. + // It comes back as a result carrying BUILD_CANCELLED, so without this the + // user's own Stop finished as BuildState.Error("Task execution failed: + // BUILD_CANCELLED"), with the enum name shown to them. + if (result?.failure == TaskExecutionResult.Failure.BUILD_CANCELLED) { + log.info("Build was cancelled by the user.") + finish(BuildState.Idle) + return@launch + } + throw RuntimeException("Task execution failed: ${result?.failure}") } if (isPluginProject) { diff --git a/app/src/test/java/com/itsaky/androidide/analytics/BuildCompletedMetricTest.kt b/app/src/test/java/com/itsaky/androidide/analytics/BuildCompletedMetricTest.kt new file mode 100644 index 0000000000..7a4671d0eb --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/analytics/BuildCompletedMetricTest.kt @@ -0,0 +1,78 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.analytics + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.analytics.gradle.BuildCompletedMetric +import com.itsaky.androidide.tooling.api.messages.BuildId +import com.itsaky.androidide.tooling.api.messages.result.BuildResult +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * What build telemetry says about a build that did not succeed (ADFA-5542). + * + * A build the user stopped and a build that broke both report `success=false`. Without the reason + * beside it the two are indistinguishable, so every build-success rate counts deliberate cancels + * as failures -- and the cancel is the one the IDE deliberately makes easy to press. + */ +@RunWith(RobolectricTestRunner::class) +class BuildCompletedMetricTest { + private fun metric( + isSuccess: Boolean, + failure: TaskExecutionResult.Failure?, + ) = BuildCompletedMetric( + buildId = BuildId.Unknown, + buildType = "assemble", + isSuccess = isSuccess, + buildResult = + BuildResult( + buildId = BuildId.Unknown, + tasks = listOf(":app:assembleDebug"), + durationMs = 1_234L, + failure = failure, + ), + ) + + @Test + fun `a cancelled build carries the reason that says so`() { + val bundle = metric(isSuccess = false, failure = TaskExecutionResult.Failure.BUILD_CANCELLED).asBundle() + + assertThat(bundle.getString("failure_reason")).isEqualTo("BUILD_CANCELLED") + // isSuccess keeps its plain meaning: the build did not succeed. The reason is what lets a + // consumer separate the user's own Stop from a broken build. + assertThat(bundle.getBoolean("success")).isFalse() + } + + @Test + fun `a build that really failed carries its own reason`() { + val bundle = metric(isSuccess = false, failure = TaskExecutionResult.Failure.BUILD_FAILED).asBundle() + + assertThat(bundle.getString("failure_reason")).isEqualTo("BUILD_FAILED") + } + + @Test + fun `a successful build carries no reason at all`() { + val bundle = metric(isSuccess = true, failure = null).asBundle() + + assertThat(bundle.containsKey("failure_reason")).isFalse() + assertThat(bundle.getBoolean("success")).isTrue() + } +} diff --git a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt index effb235731..186cfa6873 100644 --- a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt +++ b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt @@ -111,11 +111,15 @@ class EditorBuildEventListenerAnnotationTest { } @Test - fun `a build the user stopped is not reported as an error anywhere`() { - // The annotation was fixed first and the reports beside it were not, so a user who pressed - // Stop still got a red "Build failed" bar, a "Build failed" notification and an - // isSuccess=false result carrying failure text -- their own action read back to them as an - // error in every place but the chart. + fun `the message for a build the user stopped is the cancelled text`() { + // One of the four places a cancel used to be reported as an error. The others are pinned + // separately -- the notification by GradleBuildServiceNotificationStatusTest, the chart + // marker by the outcomeKind cases above. The bar itself (flashInfo rather than flashError) + // and the cancelled-sync branch in ProjectHandlerActivity are not pinned: both need a live + // activity, which is what onBuildFailed returns early without. + // + // This test was previously named for all four and asserted only this one, so deleting the + // flashInfo branch or the notification branch left it green. assertThat(listener.failureMessage(TaskExecutionResult.Failure.BUILD_CANCELLED, CANCELLED_TEXT)) .isEqualTo(CANCELLED_TEXT) } diff --git a/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceNotificationStatusTest.kt b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceNotificationStatusTest.kt new file mode 100644 index 0000000000..4327102618 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceNotificationStatusTest.kt @@ -0,0 +1,62 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.services.builder + +import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage +import com.itsaky.androidide.R +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * What the shade says about a build that did not succeed (ADFA-5542). + * + * The notification is one of the four places a cancelled build used to be reported as a failure, + * and the only one of them with no test: the chart marker and the message both had one, so the + * notification could have been reverted without a single test noticing. + */ +@RunWith(RobolectricTestRunner::class) +class GradleBuildServiceNotificationStatusTest { + private val service = GradleBuildService() + + @Test + fun `a build the user stopped says so in the shade`() { + assertThat(service.notificationStatusFor(TaskExecutionResult.Failure.BUILD_CANCELLED)) + .isEqualTo(R.string.info_build_cancelled) + } + + @Test + fun `every other failure says the build failed`() { + TaskExecutionResult.Failure.entries + .filter { it != TaskExecutionResult.Failure.BUILD_CANCELLED } + .forEach { failure -> + assertWithMessage(failure.name) + .that(service.notificationStatusFor(failure)) + .isEqualTo(R.string.build_status_failed) + } + } + + @Test + fun `a failure the server did not classify says the build failed`() { + // Reporting an unclassified failure as a cancel would put the user's name on something + // they did not do. + assertThat(service.notificationStatusFor(null)).isEqualTo(R.string.build_status_failed) + } +} diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 52e99fa8ca..9ec7372fe6 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -1162,6 +1162,7 @@ No APK found in output listing file. APK file specified does not exist: %1$s Build was cancelled by the user. + Could not stop the build. Quick Run failed. Building… Installing plugin… 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 f76315071f..c9549d8985 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 @@ -214,7 +214,17 @@ internal class ToolingApiServerImpl : IToolingApiServer { clientConfig = clientConfig, ) - RootModelBuilder.build(params, modelBuilderParams) + try { + RootModelBuilder.build(params, modelBuilderParams) + } finally { + // The sync path never cleared this on any outcome -- only shutdown() and an actual + // Stop did. So after every sync a token for a finished build sat here: the next + // Stop cancelled that dead source and answered wasEnqueued = true with no build + // running, and the check at the top of this method cancelled it again on the next + // initialize. The sibling of the same omission in executeTasks. + buildCancellationToken = null + } + notifyBuildSuccess( BuildResult( tasks = emptyList(), @@ -301,23 +311,30 @@ internal class ToolingApiServerImpl : IToolingApiServer { 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) return@runBuild TaskExecutionResult( false, notifyBuildFailure(message.buildId, message.tasks, start, error), ) + } finally { + // On both paths. Only the success path cleared it, so every failed build left a + // token behind for a source that was already finished. The next Stop then + // cancelled that dead source and answered wasEnqueued = true while the live build + // ran on, and [initialize] -- which cancels first whenever one is set -- paid for + // a build that had ended long before. + this.buildCancellationToken = null } + + notifyBuildSuccess( + result = + BuildResult( + tasks = message.tasks, + buildId = message.buildId, + durationMs = System.currentTimeMillis() - start, + ), + ) + return@runBuild TaskExecutionResult.SUCCESS } } 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 8c399e4cc7..9f1c4501ba 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.IToolingApiClient import com.itsaky.androidide.tooling.api.messages.BuildId import com.itsaky.androidide.tooling.api.messages.InitializeProjectParams +import com.itsaky.androidide.tooling.api.messages.result.BuildCancellationRequestResult import com.itsaky.androidide.tooling.api.messages.result.BuildResult import com.itsaky.androidide.tooling.api.messages.result.InitializeResult import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult @@ -128,6 +129,44 @@ class ToolingApiServerImplTest { assertThat(reported.captured.failure).isEqualTo(TaskExecutionResult.Failure.BUILD_CANCELLED) } + @Test + fun `GIVEN a sync that finished WHEN a Stop arrives THEN there is no build to cancel`() { + mockkObject(RootModelBuilder) + every { RootModelBuilder.build(any(), any()) } returns File("/does/not/exist/cache") + + val (server) = mockkToolingServer() + every { server.validateProjectDirectory(any()) } returns null + server.connect(mockk(relaxed = true)) + + server.initialize(testInitParams()).get(5, TimeUnit.SECONDS) + + // The token for the sync's own build was never cleared on any outcome, so it outlived the + // build it belonged to. A Stop pressed afterwards cancelled that dead source and answered + // "enqueued" -- telling the user a build was being stopped when none was running, and, once + // a real build had started, leaving it running while claiming otherwise. + val result = server.cancelCurrentBuild().get(5, TimeUnit.SECONDS) + + assertThat(result.wasEnqueued).isFalse() + assertThat(result.failureReason).isEqualTo(BuildCancellationRequestResult.Reason.NO_RUNNING_BUILD) + } + + @Test + fun `GIVEN a sync that failed WHEN a Stop arrives THEN there is no build to cancel`() { + mockkObject(RootModelBuilder) + every { RootModelBuilder.build(any(), any()) } throws RuntimeException("intentional failure") + + val (server) = mockkToolingServer() + every { server.validateProjectDirectory(any()) } returns null + server.connect(mockk(relaxed = true)) + + server.initialize(testInitParams()).get(5, TimeUnit.SECONDS) + + // The failing path leaked it the same way the succeeding one did. + val result = server.cancelCurrentBuild().get(5, TimeUnit.SECONDS) + + assertThat(result.wasEnqueued).isFalse() + } + @Test fun `GIVEN force sync not requested WHEN sync files are unreadable THEN sync anyway`() { val initParams = testInitParams(forceSync = false) diff --git a/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt b/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt index 3732a072d5..baf6359b9d 100644 --- a/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt +++ b/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt @@ -29,7 +29,13 @@ data class BuildResult( val tasks: List, val durationMs: Long, /** - * Why the build failed, or `null` if it succeeded. + * Why the build failed. + * + * `null` on a successful build, which this type also carries. On a failed one this server + * always fills it in -- `notifyBuildFailure` classifies the throwable and returns a + * non-null [TaskExecutionResult.Failure], and both catch paths go through it -- so a client + * seeing `null` here alongside a failure is talking to a server that does not classify, not + * to this one. Nullable on the wire for exactly that case. * * The server is the only party that can answer this: Gradle raises a * `BuildCancelledException` for a build the user stopped, and the same throwable that decides From f607f03e5ed714f77b97cf0c290f2a4a44a001d7 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 18:08:24 -0700 Subject: [PATCH 5/5] ADFA-5542: follow stage's move of finish() onto the reporter Merging stage through the stack broke this branch without a merge conflict. The cancelled-build arm added here calls finish(BuildState.Idle); stage has since moved that function onto reporter, so every other call site in the file arrived already saying reporter.finish and this one did not. Git had nothing to flag -- the two changes touch different lines -- and the branch simply stopped compiling. Fixed where the call was introduced rather than at the stack tip, so every branch above inherits it by merge instead of carrying its own copy. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt b/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt index cc2338f173..1d56122c16 100644 --- a/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt +++ b/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt @@ -99,7 +99,7 @@ class BuildViewModel( // BUILD_CANCELLED"), with the enum name shown to them. if (result?.failure == TaskExecutionResult.Failure.BUILD_CANCELLED) { log.info("Build was cancelled by the user.") - finish(BuildState.Idle) + reporter.finish(BuildState.Idle) return@launch } throw RuntimeException("Task execution failed: ${result?.failure}")