Concurrent builder reports an independent, successfully built module as skipped when the reactor halts

[slachiewicz]

Affected version

4.0.0-rc-4 through 4.0.0-rc-6, maven-4.0.x, and master. Verified by running a locally built 4.1.0-SNAPSHOT from master; the rc range is established by tag containment and code inspection, not by running the released rc binaries.

Bug description

Under the concurrent builder, when one module fails and the reactor halts, an unrelated module that was already in flight is reported as skipped — even when it completed all of its requested work successfully — and the log states a reason that is not true:

X was not built because a module it depends on failed to build.

Both halves of that sentence can be false: X may have no dependency on the failed module, and X may in fact have been built.

Reproduction

Two sibling modules with no dependency between them. slow has 4000 generated sources so its compile takes ~1.5s; fails has one source with a syntax error so its compile fails almost immediately.

root/pom.xml          <modules>slow, fails</modules>, packaging pom
root/slow/pom.xml     plain jar module, no dependencies
root/fails/pom.xml    plain jar module, no dependencies

mkdir -p slow/src/main/java/gen fails/src/main/java
for i in $(seq 0 3999); do
  printf 'package gen;\npublic class C%s { public int v() { return %s; } }\n' $i $i \
    > slow/src/main/java/gen/C$i.java
done
printf 'public class Bad implements int {}\n' > fails/src/main/java/Bad.java

mvn compile -b concurrent -T5

Result (5 runs out of 5 — it is a race, but one that is won consistently):

[INFO] --- compiler:3.13.0:compile (default-compile) @ slow ---
[INFO] --- compiler:3.13.0:compile (default-compile) @ fails ---
[INFO] Compiling 1 source file with javac ... to target/classes
[INFO] Compiling 4001 source files with javac ... to target/classes
[ERROR] .../fails/src/main/java/Bad.java:[1,29] unexpected type
[INFO] ------------------------------------------------------------------------
[INFO] Skipping slow
[INFO] slow was not built because a module it depends on failed to build.
[INFO] ------------------------------------------------------------------------

slow compiled all 4001 sources and slow/target/classes/gen contains 4000 fresh .class files (the target directories were removed before every run). It nonetheless gets ProjectSkipped and no BuildSummary.

The same project under the legacy builder (mvn compile -T5) prints no Skipping line: the in-flight project runs to completion and is reported as succeeded.

Mechanism

All references are to impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java on master.

1. fails throws a LifecycleExecutionException, so handleBuildError takes the REACTOR_FAIL_FAST branch and calls ReactorBuildStatus.halt() (line 881).

2. processStep then evaluates every remaining CREATED regular step of every project, including unrelated ones that are mid-build:

   shouldExecute = !status.isHalted() && !status.isBlackListed(step.project) && allPredecessorsExecuted; // :423

   isHalted() is now true, so each step transitions CREATED -> SKIPPED (line 452). With -X:

   [DEBUG] Skipping step BuildStep[project=...:slow, phase=ready] because the build is halted
   [DEBUG] Skipping step BuildStep[project=...:slow, phase=test-compile] because the build is halted
   [DEBUG] Skipping step BuildStep[project=...:slow, phase=package] because the build is halted
   [DEBUG] Skipping step BuildStep[project=...:slow, phase=install] because the build is halted
   [DEBUG] Skipping step BuildStep[project=...:slow, phase=deploy] because the build is halted
   

3. TEARDOWN always runs (line 416) and computes:

   allStepsExecuted &= step == projectStep || projectStep.status.get() == EXECUTED; // :535

   calculateLifecycleMappings creates before/at/after steps for every lifecycle phase regardless of the requested goal. Steps outside the requested scope are passed through BuildStep.skip(), which only sets a flag and clears the mojo list (BuildStep.java:106-109) — the step stays in the plan with status CREATED and normally executes as an empty no-op. So for mvn compile, slow's plan still holds ready, test-compile, test, package, install and deploy steps. After a halt these become SKIPPED rather than EXECUTED, allStepsExecuted is false, and with failures == null the code falls through to ProjectSkipped (line 558) instead of ProjectSucceeded (line 556).

   In other words allStepsExecuted currently means "the project traversed the entire lifecycle plan", not "the requested work completed".

4. ExecutionEventLogger.projectSkipped (impl/maven-cli/src/main/java/org/apache/maven/cling/event/ExecutionEventLogger.java:319) then prints the single hardcoded reason, because the event carries no reason of its own.

Why the legacy builder does not show this

LifecycleModuleBuilder fires ProjectSkipped before ProjectStarted and only for projects that never started (LifecycleModuleBuilder.java:91-94). The concurrent builder introduced a state that did not previously exist: started, ran its requested goals fine, then got cut off. (The message was never fully accurate even in the legacy builder — under fail-fast a halted project also gets the "a module it depends on failed" text — but it was at least limited to projects that genuinely produced nothing.)

Provenance

ProjectSkipped was added to BuildPlanExecutor by 3706aa1, "[MNG-8670] Fix concurrent builder missing/wrong project events" (#2251, for #9636), first released in 4.0.0-rc-4. Before that commit TEARDOWN fired ProjectSucceeded unconditionally, so this is a regression of the reporting for halted-but-complete projects rather than a pre-existing gap.

Impact on the IT suite

This makes MavenITmng8648ProjectEventsTest flaky. The test builds a five-module reactor with compile -b concurrent -T5 where subproject-c fails on purpose, and asserts:

verifier.verifyTextInLog("org.apache.maven.its.mng8648:subproject-b:jar:1-SNAPSHOT ProjectSucceeded"); // :52

subproject-b does not depend on subproject-c (only subproject-d does), so whether b gets ProjectSucceeded or ProjectSkipped depends on whether c fails before b's empty compile finishes. It went the wrong way on integration-tests (macos-latest, 21) in https://github.com/apache/maven/actions/runs/31221127749/job/93006825497 (attempt 1 of that run; attempt 2 was green, which is why the run now shows as successful):

subproject-b:jar:1-SNAPSHOT ProjectStarted
[INFO] --- compiler:3.13.0:compile (default-compile) @ subproject-b ---
subproject-c:jar:1-SNAPSHOT ProjectFailed
[INFO] No sources to compile
subproject-b:jar:1-SNAPSHOT ProjectSkipped
[INFO] Skipping subproject-b
[INFO] subproject-b was not built because a module it depends on failed to build.

Only 1 of 12 integration-test matrix jobs failed in that run.

Suggested direction

1. Event. Base the outcome on whether the project's in-scope work completed, i.e. compute allStepsExecuted over steps that were not skip()-flagged (or equivalently require every mojo-bearing step to have reached EXECUTED). A project halted with in-scope mojos still pending genuinely did not complete and should stay ProjectSkipped; one that ran everything asked of it should be ProjectSucceeded. This keeps the MNG-8670 use case intact — subproject-d, which really was never built, still gets ProjectStarted + ProjectSkipped — and it matches what the MNG-8670 IT itself asserts for subproject-b.

2. Message. ProjectSkipped now has at least two causes (blacklisted because a dependency failed vs. reactor halted). The logger cannot tell them apart today and always claims the first. The reason needs to reach ExecutionEventLogger, so a halted project reads "the build was halted after an earlier failure" instead.

3. IT. Fixing the event alone does not fully deflake MavenITmng8648ProjectEventsTest: if c fails before b's steps are even scheduled, b is legitimately skipped and the assertion still fails. The test needs an ordering guarantee — e.g. make subproject-c fail slowly so b reliably finishes first — rather than a loosened assertion.

I am happy to put up a PR for 1 and 2 if the direction looks right.

[slachiewicz]

Two existing issues ask for the same log line to be improved, from a different angle than this one. Recording them here so the wording is not changed twice in opposite directions:

• [MNG-8069] Message "This project has been banned from the build due to previous failures." is too unspecific #10800 (MNG-8069) — the message is too unspecific; it should name the project that actually failed. PR [MNG-8069] add failing projects if a project is banned from reactor #1435 implements that, still open, and it predates the module restructure so it patches maven-embedder/.../cli/event/ExecutionEventLogger.java rather than today's impl/maven-cli/.../cling/event/ExecutionEventLogger.java.
• [MNG-8155] Reword "This project has been banned from the build due to previous failures." #10609 (MNG-8155) — where the current wording came from, via [MNG-8155] Improve not built error message #1599. Still open even though that PR is merged.

Those two are about naming the culprit. This issue is about the sentence being false in the first place when nothing the project depends on failed. They compose: "X was not built because Y, which it depends on, failed to build" for the dependency case, and a separate sentence for the halted case.

Also in the same area, for anyone triaging BuildPlanExecutor: #12585 and #12599 (concurrent writes to MavenSession, leaked thread pool) and PR #12680 (races in artifact resolution and phase ordering). None of them overlaps with the TEARDOWN outcome logic this issue is about.