Concurrent executor ignores java.lang.Error subclasses

[HuzaifaChaudary]

Closes #10415 , which is MNG-8678 .

the concurrent builder wraps every step in catch (Exception e) , so a step that throws an
Error is not caught at all . step.status never moves off SCHEDULED and step.exception
stays null , so isDone() is false , the project's teardown step never becomes eligible ,
handleBuildError never runs and nothing is added to the result . the build finishes with an
empty exception list and prints BUILD SUCCESS .

the worker thread dies , but PhasingExecutor.execute decrements the active count in a
finally , so nothing hangs either . it just quietly succeeds .

the rest of the machinery already handles this . handleBuildError has a branch whose comment
says fail fast on RuntimeExceptions, Errors and "other" Throwables , and the failures list
is already List<Throwable> . only the plumbing feeding it was narrowed to Exception . the
sequential builder catches Throwable , which is the inconsistency the report names .

so this widens the two catches and the field they write to . six lines .

i left BuildContext.execute() alone even though it also catches only Exception , because
DefaultMaven catches RuntimeException , so an error on the main thread still propagates
rather than turning into a success . that path does not produce this symptom .

two tests . one throws an Error from a build step and asserts it reaches
getResult().getExceptions() , which fails without the change with expected the error to be
recorded, but got: [] . the other throws a RuntimeException , which already worked , so a
change that was too wide would show up there .

mvn -pl impl/maven-core test is 683 passing , 0 failures , and spotless and checkstyle are
bound in that run and clean .

i have not run the core integration tests , they live in another repo and need a full
distribution build .

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[renechoi]

Reproduced on 6eba735, JDK 21: test-only on master gives expected the error to be recorded, but got: []; branch is 683/0 on mvn -pl impl/maven-core test.

One gap: the after:* cleanup catch (L404) is not covered here, and it is what adds a second failure to the same project. Once failures.size() > 1, executeStep wraps them into a LifecycleExecutionException, so handleBuildError's t instanceof RuntimeException || !(t instanceof Exception) is false. Measured: halted=true for a bare Error, halted=false (only blacklisted, -fae) once wrapped. Deciding halt from failures keeps it.

[HuzaifaChaudary]

you are right . once there is more than one failure the wrapper is a checked exception , so the error inside it stops counting as fatal and under -fae the project is only blacklisted . pushed a fix that reads the halt from the failures rather than from the wrapper . with a single failure it comes out to the same check as before , so nothing else moves . the new test fails without it .

being straight about the test though , it does not drive a real after:* failure . a step with no mojo cannot be made to throw in this harness , so the second failure is put on the after:validate step the same way the cleanup catch stores it . the collecting , the wrapping and the halt decision all run for real , but the cleanup step is not what raises it .

[gnodet]

Solid fix. The bug is well-analyzed, the code change is minimal and precisely targeted, and the tests cover the important cases.

The core insight — that catch (Exception) in processStep silently drops Error subclasses, leaving step.status stuck at SCHEDULED and producing a false BUILD SUCCESS — is correct. The sequential builder already catches Throwable, making this an inconsistency.

The isFatal(List<Throwable>) extraction is the interesting part: it addresses a second-order problem that @renechoi flagged — when multiple failures get wrapped in a LifecycleExecutionException (a checked exception), the original instanceof check on the wrapper masks any Error buried inside. Reading fatality from the individual failures instead of the wrapper is the right call.

Verified:

• Single Error: isFatal=true, reactor halted (was silently succeeding before)
• Single checked Exception: isFatal=false, no halt, same as before
• Wrapped Error + cleanup failure: isFatal=true, reactor halted (was downgraded to blacklist-only before)
• handleBuildError API change is safe: BuildContext is package-private, no external subclasses
• Test coverage pins all three scenarios

Category: bug
Suggested milestone: 4.1.0 (targets master)
Backport: not needed — concurrent builder is 4.x only

This review was generated by an AI agent, Hermès on behalf of @gnodet.