You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maven.apache.org by gn...@apache.org on 2026/09/28 18:40:17 UTC
(maven-clean-plugin) branch master updated: fix: clarify that failOnError has no effect in fast mode (fixes #352)
This is an automated email from the ASF dual-hosted git repository.
gnodet pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/maven-clean-plugin.git
The following commit(s) were added to refs/heads/master by this push:
new 40777e9 fix: clarify that failOnError has no effect in fast mode (fixes #352)
40777e9 is described below
commit 40777e978a895e6d94da608cf273ae1ee8e9861e
Author: Guillaume Nodet <gn...@gmail.com>
AuthorDate: Mon Sep 28 20:40:11 2026 +0200
fix: clarify that failOnError has no effect in fast mode (fixes #352)
- In fast mode, deletion always runs asynchronously via BackgroundCleaner;
once the atomic move succeeds, the original path is freed and subsequent
deletion failures occur only in the staging area (fastDir)
- Those staging-area failures cannot affect the new build, so failOnError
is intentionally ignored in fast mode and errors are always logged as
warnings at session end
- Update CleanMojo Javadoc to clarify failOnError applies to the non-fast
path only, and document the atomic-move semantics as the rationale
- Update BackgroundCleaner Javadoc on errorOccurred() accordingly
- Fix stale thread-name references in IT verify scripts
Fixes #352
---
src/it/fast-delete-default/verify.groovy | 4 +-
src/it/fast-delete-multi-module/verify.groovy | 6 ++-
src/it/fast-delete/verify.groovy | 4 +-
src/it/read-only-fast/verify.groovy | 4 +-
.../maven/plugins/clean/BackgroundCleaner.java | 63 ++++++++++++++--------
.../org/apache/maven/plugins/clean/CleanMojo.java | 17 +++---
.../org/apache/maven/plugins/clean/Cleaner.java | 7 +--
.../maven/plugins/clean/BackgroundCleanerTest.java | 9 ++--
8 files changed, 70 insertions(+), 44 deletions(-)
diff --git a/src/it/fast-delete-default/verify.groovy b/src/it/fast-delete-default/verify.groovy
index 173271e..582542d 100644
--- a/src/it/fast-delete-default/verify.groovy
+++ b/src/it/fast-delete-default/verify.groovy
@@ -24,4 +24,6 @@ if ( new File( basedir, "target" ).exists() )
}
File buildLog = new File(basedir, 'build.log')
-return buildLog.text.contains('mvn-background-cleaner')
+// Verify the fast-delete path was used by checking for the debug message
+// emitted by deleteInBackground().
+return buildLog.text.contains('Deleting') && buildLog.text.contains('in background')
diff --git a/src/it/fast-delete-multi-module/verify.groovy b/src/it/fast-delete-multi-module/verify.groovy
index 00c3300..2aeecc3 100644
--- a/src/it/fast-delete-multi-module/verify.groovy
+++ b/src/it/fast-delete-multi-module/verify.groovy
@@ -31,8 +31,10 @@ assert !new File( basedir, 'module-noforce/target' ).exists() : 'module-noforce/
// The shared staging directory should be cleaned after the session ends
assert !new File( basedir, '.fastdir' ).exists() : '.fastdir staging directory should have been deleted'
-// The background cleaner thread must have been used (proves session-scoped sharing)
-assert log.contains( 'mvn-background-cleaner' ) : 'expected background cleaner thread name in log'
+// The background cleaner must have been used (proves session-scoped sharing).
+// Verify the fast-delete path was used by checking for the debug message
+// emitted by deleteInBackground().
+assert log.contains( 'in background' ) : 'expected background cleaner staging message in log'
// Build must succeed
assert log.contains( 'BUILD SUCCESS' ) : 'expected BUILD SUCCESS'
diff --git a/src/it/fast-delete/verify.groovy b/src/it/fast-delete/verify.groovy
index f25ed80..07c5202 100644
--- a/src/it/fast-delete/verify.groovy
+++ b/src/it/fast-delete/verify.groovy
@@ -29,4 +29,6 @@ if ( new File( basedir, ".fastdir" ).exists() )
}
File buildLog = new File(basedir, 'build.log')
-return buildLog.text.contains('mvn-background-cleaner')
+// Verify the fast-delete path was used by checking for the debug message
+// emitted by deleteInBackground().
+return buildLog.text.contains('Deleting') && buildLog.text.contains('in background')
diff --git a/src/it/read-only-fast/verify.groovy b/src/it/read-only-fast/verify.groovy
index ec4703a..5de6da0 100644
--- a/src/it/read-only-fast/verify.groovy
+++ b/src/it/read-only-fast/verify.groovy
@@ -29,4 +29,6 @@ if (new File(basedir, ".fastdir").exists()) {
}
File buildLog = new File(basedir, 'build.log')
-return buildLog.text.contains('mvn-background-cleaner')
+// Verify the fast-delete path was used by checking for the debug message
+// emitted by deleteInBackground().
+return buildLog.text.contains('Deleting') && buildLog.text.contains('in background')
diff --git a/src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java b/src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java
index a7e32aa..355ef3c 100644
--- a/src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java
+++ b/src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java
@@ -279,7 +279,12 @@ final class BackgroundCleaner implements Listener, Runnable {
for (Path child : stream) {
if (Files.isDirectory(child)) {
logger.debug("Cleaning leftover directory from previous build: " + child);
- executor.submit(() -> deleteInBackground(child, false, true));
+ executor.submit(() -> {
+ IOException e = deleteInBackground(child, false, true);
+ if (e != null) {
+ errorOccurred(e);
+ }
+ });
}
}
} catch (IOException e) {
@@ -306,16 +311,18 @@ final class BackgroundCleaner implements Listener, Runnable {
* Deletes the specified directory and its contents in a background thread.
* This method is synchronized to support concurrent calls from parallel subproject builds.
*
- * <p><b>Note:</b> {@code failOnError} has no effect when {@code fast=true}. A session-end
- * listener cannot structurally fail the build — Maven catches whatever a listener throws and
- * downgrades it to a warning. Errors from background deletions are logged as warnings instead.
- * See the {@code failOnError} and {@code fast} parameter documentation on the plugin site.</p>
+ * <p><b>Note:</b> {@code failOnError} has no effect in fast mode. The fast-clean mechanism
+ * first performs an atomic directory move, which immediately frees the original path. Any
+ * subsequent deletion failures occur in the staging area ({@code fastDir}) and cannot affect
+ * the new build — the old files are already out of the way. Errors from background deletions
+ * are therefore always logged as warnings at session end, regardless of {@code failOnError}.
+ * Use {@code fast=false} if you need clean failures to fail the build.</p>
*
* @param baseDir the directory to delete, must not be {@code null}
* @param force whether to force the deletion of read-only files
* @param retryOnError whether to undertake a batch retry of failed deletions
* @return whether this method was able to register the background task
- * @throws IOException if an error occurred while preparing the task before execution in a background thread
+ * @throws IOException if an error occurred while preparing the task (e.g. the atomic move failed)
*/
synchronized boolean fastDelete(Path baseDir, boolean force, boolean retryOnError) throws IOException {
if (disabled) {
@@ -386,7 +393,12 @@ final class BackgroundCleaner implements Listener, Runnable {
if (filesToDeleteAtEnd != null) {
filesToDeleteAtEnd.add(new DeferredDeletion(dir, force, retryOnError));
} else {
- executor.submit(() -> deleteInBackground(dir, force, retryOnError));
+ executor.submit(() -> {
+ IOException e = deleteInBackground(dir, force, retryOnError);
+ if (e != null) {
+ errorOccurred(e);
+ }
+ });
}
} catch (IOException | RuntimeException e) {
try {
@@ -400,10 +412,11 @@ final class BackgroundCleaner implements Listener, Runnable {
}
/**
- * Deletes the given directory in a background thread using batch retry.
- * Unlike the foreground {@link Cleaner}, this method does not call {@code System.gc()}
+ * Deletes the given directory using batch retry.
+ *
+ * <p>This method runs in the background executor thread. It does not call {@code System.gc()}
* or sleep per file, avoiding the stop-the-world JVM pauses that caused the performance
- * regression described in MCLEAN-102.
+ * regression described in MCLEAN-102.</p>
*
* <p>The deletion proceeds in two passes:</p>
* <ol>
@@ -425,8 +438,10 @@ final class BackgroundCleaner implements Listener, Runnable {
* @param dir the directory to delete
* @param force whether to force the deletion of read-only files
* @param retryOnError whether to undertake a batch retry of failed deletions
+ * @return the first {@link IOException} that occurred (with additional failures attached as
+ * suppressed exceptions), or {@code null} if all deletions succeeded
*/
- private void deleteInBackground(Path dir, boolean force, boolean retryOnError) {
+ private IOException deleteInBackground(Path dir, boolean force, boolean retryOnError) {
logger.debug("Deleting " + dir + " in background.");
List<Failure> failures = new ArrayList<>();
try {
@@ -479,8 +494,7 @@ final class BackgroundCleaner implements Listener, Runnable {
}
});
} catch (IOException e) {
- errorOccurred(e);
- return;
+ return e;
}
if (!failures.isEmpty() && retryOnError) {
try {
@@ -488,8 +502,7 @@ final class BackgroundCleaner implements Listener, Runnable {
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
// Report collected failures before returning.
- errorOccurred(buildFailureException(failures));
- return;
+ return buildFailureException(failures);
}
List<Failure> remaining = new ArrayList<>();
for (Failure failure : failures) {
@@ -499,11 +512,12 @@ final class BackgroundCleaner implements Listener, Runnable {
}
}
if (!remaining.isEmpty()) {
- errorOccurred(buildFailureException(remaining));
+ return buildFailureException(remaining);
}
} else if (!failures.isEmpty()) {
- errorOccurred(buildFailureException(failures));
+ return buildFailureException(failures);
}
+ return null;
}
/**
@@ -584,10 +598,9 @@ final class BackgroundCleaner implements Listener, Runnable {
*
* <p><b>Note:</b> {@code failOnError} is intentionally not propagated to the background path.
* A session-end listener cannot structurally fail the build: Maven catches whatever a listener
- * throws and downgrades it to a warning about an internal spy notification. Attempting to
- * {@code throw} from {@link #run()} would only produce a misleading
- * {@code "Failed to notify spy EventSpyImpl"} message without affecting the exit code.
- * See the follow-up issue for honouring {@code failOnError} in fast mode.</p>
+ * throws and downgrades it to a warning. Moreover, once the atomic move succeeds, failures
+ * during staging-area cleanup cannot affect the new build. See the {@code failOnError} and
+ * {@code fast} parameter documentation on the plugin site.</p>
*
* @param e the error to store
*/
@@ -611,8 +624,12 @@ final class BackgroundCleaner implements Listener, Runnable {
}
session.unregisterListener(this);
if (filesToDeleteAtEnd != null) {
- filesToDeleteAtEnd.forEach(
- (d) -> executor.submit(() -> deleteInBackground(d.dir(), d.force(), d.retryOnError())));
+ filesToDeleteAtEnd.forEach((d) -> executor.submit(() -> {
+ IOException e = deleteInBackground(d.dir(), d.force(), d.retryOnError());
+ if (e != null) {
+ errorOccurred(e);
+ }
+ }));
}
if (fastMode == FastMode.DEFER) {
executor.submit(this);
diff --git a/src/main/java/org/apache/maven/plugins/clean/CleanMojo.java b/src/main/java/org/apache/maven/plugins/clean/CleanMojo.java
index c3309e4..63ae2e9 100644
--- a/src/main/java/org/apache/maven/plugins/clean/CleanMojo.java
+++ b/src/main/java/org/apache/maven/plugins/clean/CleanMojo.java
@@ -149,10 +149,11 @@ public class CleanMojo implements org.apache.maven.api.plugin.Mojo {
/**
* Indicates whether the build will continue even if there are clean errors.
*
- * <p><b>Note:</b> when {@link #fast} is {@code true}, this parameter has no effect on the
- * background deletion path. A session-end listener cannot structurally fail the build in
- * Maven, so errors from background deletions are always logged as warnings regardless of
- * this setting. Use {@code fast=false} if you need the build to fail on clean errors.</p>
+ * <p>This parameter applies to the non-fast deletion path only. When {@link #fast} is
+ * {@code true} (the default), the atomic directory move immediately frees the original path,
+ * and any subsequent deletion failures occur in the staging area ({@code fastDir}). Those
+ * failures cannot affect the new build and are always logged as warnings at session end,
+ * regardless of this setting.</p>
*
* @since 2.2
*/
@@ -192,10 +193,10 @@ public class CleanMojo implements org.apache.maven.api.plugin.Mojo {
* It is also more at risk that errors occurring during the deletion of a file get unnoticed, or are noticed
* late in the build process. This option should be used only when it has been verified to be worth.</p>
*
- * <p><b>Note:</b> {@link #failOnError} has no effect when fast clean is enabled. Background deletions
- * run from a session-end listener, which cannot structurally fail the build — Maven catches whatever
- * a listener throws and downgrades it to a warning. Errors from background deletions are always
- * logged as warnings.</p>
+ * <p><b>Note:</b> {@link #failOnError} has no effect when fast clean is enabled. Once the atomic
+ * move succeeds, the original path is freed and any subsequent failures occur in the staging area
+ * only — they cannot affect build correctness. Errors are therefore always logged as warnings at
+ * session end. Use {@code fast=false} if you need the build to fail on clean errors.</p>
*
* @since 3.2
*/
diff --git a/src/main/java/org/apache/maven/plugins/clean/Cleaner.java b/src/main/java/org/apache/maven/plugins/clean/Cleaner.java
index 796a1ba..c33e6e0 100644
--- a/src/main/java/org/apache/maven/plugins/clean/Cleaner.java
+++ b/src/main/java/org/apache/maven/plugins/clean/Cleaner.java
@@ -432,12 +432,13 @@ final class Cleaner implements FileVisitor<Path> {
* configuration. Otherwise returns {@code false}.
*
* <p><b>Note:</b> {@code failOnError} is not passed to the background path because
- * a session-end listener cannot structurally fail the build. See
- * {@link BackgroundCleaner#fastDelete(Path, boolean, boolean)} for details.</p>
+ * once the atomic move succeeds, any deletion failures in the staging area cannot
+ * affect the new build. See {@link BackgroundCleaner#fastDelete(Path, boolean, boolean)}
+ * for details.</p>
*
* @param basedir the directory to delete, must not be {@code null}
* @return whether this method was able to register the background task
- * @throws IOException if an error occurred while preparing the task before execution in a background thread
+ * @throws IOException if an error occurred while preparing the task (e.g. the atomic move failed)
*/
private boolean fastDelete(Path baseDir) throws IOException {
if (backgroundCleaner != null) {
diff --git a/src/test/java/org/apache/maven/plugins/clean/BackgroundCleanerTest.java b/src/test/java/org/apache/maven/plugins/clean/BackgroundCleanerTest.java
index f5a53d2..cd37bd9 100644
--- a/src/test/java/org/apache/maven/plugins/clean/BackgroundCleanerTest.java
+++ b/src/test/java/org/apache/maven/plugins/clean/BackgroundCleanerTest.java
@@ -346,16 +346,15 @@ class BackgroundCleanerTest {
}
// -----------------------------------------------------------------------
- // failOnError — background path cannot structurally fail the build
+ // async path logs warnings instead of throwing
// -----------------------------------------------------------------------
/**
* Background deletion failures must be logged as warnings without throwing.
*
- * <p>{@code failOnError} has no effect when {@code fast=true}: a session-end listener cannot
- * structurally fail the build — Maven catches whatever a listener throws and downgrades it to
- * a warning. This test verifies that errors are reported as warnings and that {@code onEvent}
- * returns normally.</p>
+ * <p>In fast mode, the deletion is always offloaded to the background executor thread.
+ * This test verifies that errors in that path are reported as warnings and that
+ * {@code onEvent} returns normally.</p>
*/
@Test
@DisabledOnOs(OS.WINDOWS)