ADFA-5542: let the server say a build was cancelled

[davidschachterADFA]

PR 14 on the carousel stack. Base is feature/ADFA-5554-longer-tooltip-hold (#1806); review that first.

Read the second commit, not the first. The first took an approach the review dismantled; the second replaces it and is what this PR is. Both are kept so the review that found the problem still reads against the code it was given.

The defect

EditorBuildEventListener held cancelRequested, a bare boolean meaning "a cancel happened recently". The only thing tying it to the build it belonged to was the order two main-thread runnables happened to run in — onBuildCancelRequested inline from the UI thread, prepareBuild posted from the build's thread. A cancel arriving between the post and its run was cleared by it, and the build the user stopped came back annotated BUILD_FAILED.

The fix: stop guessing, read the answer

The tooling server already knows, exactly and per build:

// ToolingApiServerImpl.kt
private fun getTaskFailureType(error: Throwable): Failure =
    when (error) {
        is BuildException -> BUILD_FAILED
        is BuildCancelledException -> BUILD_CANCELLED
        ...

It computed that, handed it to the caller of executeTasks, and notified the client with a BuildResult carrying tasks, an id and a duration — everything except the answer. BuildResult carries failure now, the same value from the same call, and the listener reads it.

#	Site	Change
F01	BuildResult	+ val failure: TaskExecutionResult.Failure? = null
F02	ToolingApiServerImpl	both failure sites classify the throwable once and use it for the BuildResult and the return value
F03	EventListener.onBuildFailed	(tasks, failure)
F04	EditorBuildEventListener	outcomeKind(failure) — BUILD_CANCELLED iff the server said so
F05	EventListener	onBuildCancelRequested removed, with its call in cancelCurrentBuild

What this deletes

The first commit's whole apparatus: runningBuildId, the @Volatile it needed and its non-atomic compare-and-clear, cancelledBuildId, forget(), the build id on the outcome callbacks. annotatedBuild goes back to a boolean cleared per build above the activity check — where it was, and where it belongs. Keying it to a build id existed only to avoid a reset, and the reset was never the problem; the cancel flag's reset was, and that flag is gone.

161 insertions against 191 deletions — net shorter than the code it replaces, fixing more.

onBuildCancelRequested is removed rather than left as a no-op: it existed solely so the listener could tell a cancel from a failure, and it is now shown that instead of being told it. That un-does one member ADFA-5509 (#1792) added, lower in this stack. The invariant that came with it — no defaulted members on EventListener, so the compiler asks every implementer — stays, and still guards the rest.

Tests

Test	Fails without the fix
GIVEN a build the user stopped WHEN it fails THEN the client is told it was a cancel (server)	yes — dropping failure from the notified BuildResult fails it, and nothing else. This is the regression test, on the server, because that is where the fix is
a failure reaches the listener with the server's reason for it (wrapper)	asserts the argument: a wrapper forwarding the call and dropping the reason would be the old defect one layer in, with no signature to complain
every other reason the server gives is a failure	the substance of the mapping — a dropped connection or an unsupported Gradle version must not read as the user stopping something
a failure the server did not classify is a failure	null must not become a cancel
the server saying a build was cancelled is what marks it cancelled	the positive case
preparing a build clears a stale pairing, even with no activity attached	restored; the clear sits above the activity check, so moving it below fails this

:app:testV8DebugUnitTest, :idetooltips:testV8DebugUnitTest, :subprojects:tooling-api-impl:test and spotlessCheck green.

A pre-existing break, fixed in passing

:subprojects:tooling-api-impl's test source has not compiled since ADFA-2784 added buildId to InitializeProjectParams — testInitParams never passed it. It is broken on stage too, and no workflow runs any :subprojects: tests, which is why nothing said so. One parameter here so this PR's server test can run; the CI gap wants a ticket of its own.

Review findings addressed

Every finding against the first commit is either fixed or no longer has anything to attach to: the Stop control racing prepareBuild, the --scan retry reusing a BuildId, the inert regression test, the untested forget(), the annotatedBuildId doc claim, the volatile read-modify-write, the dead @VisibleForTesting, the duplicated comparison, and the outcomeKind KDoc that described a Boolean.

The branch name still says cancel-by-build-id, from the approach that is gone. Left alone rather than churn an open PR's ref.

🤖 Generated with Claude Code

https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[davidschachterADFA]

Second review round — both findings fixed

Pushed as 29e07894d.

The sibling sweep was not done, and the review was right to say so. The chart marker was fixed and the three reports beside it still said "failed": a red flashError bar, a "Build failed" notification in the shade, and an isSuccess=false result carrying failure text posted to every plugin listener. A user who pressed Stop had their own deliberate action read back to them as an error in every place but one. The argument this PR gave for the marker applies verbatim to all four, and failure was already in hand at each. Fixed at all four.

The plugin API genuinely cannot express a cancel — IdeServices.onBuildFailed takes an error string and nothing else — so the message is the whole of what a plugin can be told. That limit is now stated in the code rather than left as an omission.

The regression test drove the wrong path. It exercised initialize(), while the ticket's defect travels executeTasks(); both sites had the same two-line edit, so deleting one left the suite green. Rather than stand up a live ProjectConnection for the second, the two are now one call: notifyBuildFailure classifies the throwable, notifies the client and returns the answer to its caller. A site can no longer report a failure without saying which, or tell the client one thing and its caller another. That leaves one place to get this wrong, and the existing test covers it — dropping the field still fails it and nothing else.

On the Spotless mixing

Also flagged: c6d6393d5 carries a whole-file ktlint reformat of the server test inside a behavioural commit, which CLAUDE.md says to commit standalone. Correct, and I have not rewritten it — that commit is pushed and has been reviewed against, and re-writing it to tidy the diff would throw away the history the review reads against for no behavioural gain. Flagging it here instead so a reviewer knows which part of that 58-line diff is formatting: everything except the buildId fix and the new test.

[davidschachterADFA]

Superseded by #1812, which merges all of the carousel work onto current stage as a single change.

Closing rather than leaving open so review effort is not split: stage moved twice today (ADFA-5514 squash-merged, plus seven other commits), the stack's lower branches had diverged from it, and reviewing these individually meant reviewing against a base that no longer exists.

Nothing is lost. This branch is untouched, the commits and their messages are in #1812's history, and reopening is a click if that turns out to be the wrong call.

#1812 carries the ticket-by-ticket detail, the five conflict resolutions the ADFA-5514 squash forced — those compile either way, so they are the part worth reviewing hardest — and a device pass on a Pixel 6 Pro covering the daemon plot, the build markers and all three chart pages.