[MBUILDCACHE-67] Bugfix artifact restoration error not handled properly

src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java

@@ -24,12 +24,15 @@
 
 import java.io.File;
 import java.nio.file.Path;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.apache.commons.lang3.ArrayUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.maven.SessionScoped;
+import org.apache.maven.buildcache.artifact.ArtifactRestorationReport;
 import org.apache.maven.buildcache.checksum.MavenProjectInput;
 import org.apache.maven.buildcache.xml.Build;
 import org.apache.maven.buildcache.xml.CacheConfig;
@@ -109,8 +112,9 @@ public void execute(
             // Forked execution should be thought as a part of originating mojo internal implementation
             // If forkedExecution is detected, it means that originating mojo is not cached so forks should rerun too
             boolean forkedExecution = lifecyclePhasesHelper.isForkedProject(project);
+            List<MojoExecution> cleanPhase = null;
             if (source == Source.LIFECYCLE && !forkedExecution) {
-                List<MojoExecution> cleanPhase = lifecyclePhasesHelper.getCleanSegment(project, mojoExecutions);
+                cleanPhase = lifecyclePhasesHelper.getCleanSegment(project, mojoExecutions);
                 for (MojoExecution mojoExecution : cleanPhase) {
                     mojoExecutionRunner.run(mojoExecution);
                 }
@@ -128,8 +132,12 @@ public void execute(
             boolean restorable = result.isSuccess() || result.isPartialSuccess();
             boolean restored = result.isSuccess(); // if partially restored need to save increment
             if (restorable) {
-                restored &= restoreProject(result, mojoExecutions, mojoExecutionRunner, cacheConfig);
-            } else {
+                CacheRestorationStatus cacheRestorationStatus =
+                        restoreProject(result, mojoExecutions, mojoExecutionRunner, cacheConfig);
+                restored &= CacheRestorationStatus.SUCCESS == cacheRestorationStatus;
+                executeExtraCleanPhaseIfNeeded(cacheRestorationStatus, cleanPhase, mojoExecutionRunner);
+            }
+            if (!restored) {
                 for (MojoExecution mojoExecution : mojoExecutions) {
                     if (source == Source.CLI
                             || mojoExecution.getLifecyclePhase() == null
@@ -155,6 +163,31 @@ public void execute(
         }
     }
 
+    /**
+     * Cache configuration could demand to restore some files in the project directory (generated sources or even arbitrary content)
+     * If an error occurs during or after this kind of restoration AND a clean phase was required in the build :
+     * we execute an extra clean phase to remove any potential partially restored files
+     *
+     * @param cacheRestorationStatus the restoration status
+     * @param cleanPhase clean phase mojos
+     * @param mojoExecutionRunner mojo runner
+     * @throws LifecycleExecutionException
+     */
+    private void executeExtraCleanPhaseIfNeeded(
+            final CacheRestorationStatus cacheRestorationStatus,
+            List<MojoExecution> cleanPhase,
+            MojoExecutionRunner mojoExecutionRunner)
+            throws LifecycleExecutionException {
+        if (CacheRestorationStatus.FAILURE_NEEDS_CLEAN == cacheRestorationStatus
+                && cleanPhase != null
+                && !cleanPhase.isEmpty()) {
+            LOGGER.info("Extra clean phase is executed as cache could be partially restored.");
+            for (MojoExecution mojoExecution : cleanPhase) {
+                mojoExecutionRunner.run(mojoExecution);
+            }
+        }
+    }
+
     private Source getSource(List<MojoExecution> mojoExecutions) {
         if (mojoExecutions == null || mojoExecutions.isEmpty()) {
             return null;
@@ -167,7 +200,7 @@ private Source getSource(List<MojoExecution> mojoExecutions) {
         return Source.LIFECYCLE;
     }
 
-    private boolean restoreProject(
+    private CacheRestorationStatus restoreProject(
             CacheResult cacheResult,
             List<MojoExecution> mojoExecutions,
             MojoExecutionRunner mojoExecutionRunner,
@@ -183,14 +216,34 @@ private boolean restoreProject(
             final List<MojoExecution> cachedSegment =
                     lifecyclePhasesHelper.getCachedSegment(project, mojoExecutions, build);
 
-            boolean restored = cacheController.restoreProjectArtifacts(cacheResult);
-            if (!restored) {
+            // Verify cache consistency for cached mojos
+            LOGGER.debug("Verify consistency on cached mojos");
+            Set<MojoExecution> forcedExecutionMojos = new HashSet<>();
+            for (MojoExecution cacheCandidate : cachedSegment) {
+                if (cacheController.isForcedExecution(project, cacheCandidate)) {
+                    forcedExecutionMojos.add(cacheCandidate);
+                } else {
+                    if (!verifyCacheConsistency(
+                            cacheCandidate, build, project, session, mojoExecutionRunner, cacheConfig)) {
+                        LOGGER.info("A cached mojo is not consistent, continuing with non cached build");
+                        return CacheRestorationStatus.FAILURE;
+                    }
+                }
+            }
+
+            // Restore project artifacts
+            ArtifactRestorationReport restorationReport = cacheController.restoreProjectArtifacts(cacheResult);
+            if (!restorationReport.isSuccess()) {
                 LOGGER.info("Cannot restore project artifacts, continuing with non cached build");
-                return false;
+                return restorationReport.isRestoredFilesInProjectDirectory()
+                        ? CacheRestorationStatus.FAILURE_NEEDS_CLEAN
+                        : CacheRestorationStatus.FAILURE;
             }
 
+            // Execute mandatory mojos (forced by configuration)
+            LOGGER.debug("Execute mandatory mojos in the cache segment");
             for (MojoExecution cacheCandidate : cachedSegment) {
-                if (cacheController.isForcedExecution(project, cacheCandidate)) {
+                if (forcedExecutionMojos.contains(cacheCandidate)) {
                     LOGGER.info(
                             "Mojo execution is forced by project property: {}",
                             cacheCandidate.getMojoDescriptor().getFullGoalName());
@@ -206,31 +259,20 @@ private boolean restoreProject(
                     //        org.apache.maven.api.MojoExecution.class, new DefaultMojoExecution(cacheCandidate));
                     mojoExecutionRunner.run(cacheCandidate);
                 } else {
-                    restored = verifyCacheConsistency(
-                            cacheCandidate, build, project, session, mojoExecutionRunner, cacheConfig);
-                    if (!restored) {
-                        break;
-                    }
-                }
-            }
-
-            if (!restored) {
-                // cleanup partial state
-                project.getArtifact().setFile(null);
-                project.getArtifact().setResolved(false);
-                mojoListener.remove(project);
-                // build as usual
-                for (MojoExecution mojoExecution : cachedSegment) {
-                    mojoExecutionRunner.run(mojoExecution);
+                    LOGGER.info(
+                            "Skipping plugin execution (cached): {}",
+                            cacheCandidate.getMojoDescriptor().getFullGoalName());
                 }
             }
 
+            // Execute mojos after the cache segment
+            LOGGER.debug("Execute mojos post cache segment");
             List<MojoExecution> postCachedSegment =
                     lifecyclePhasesHelper.getPostCachedSegment(project, mojoExecutions, build);
             for (MojoExecution mojoExecution : postCachedSegment) {
                 mojoExecutionRunner.run(mojoExecution);
             }
-            return restored;
+            return CacheRestorationStatus.SUCCESS;
         } finally {
             mojoExecutionScope.exit();
         }
@@ -263,13 +305,14 @@ private boolean verifyCacheConsistency(
 
                 if (consistent) {
                     long elapsed = System.currentTimeMillis() - createdTimestamp;
-                    LOGGER.info("Skipping plugin execution (reconciled in {} millis): {}", elapsed, fullGoalName);
-                }
 
-                if (LOGGER.isDebugEnabled()) {
                     LOGGER.debug(
-                            "Checked {}, resolved mojo: {}, cached params: {}", fullGoalName, mojo, completedExecution);
+                            "Plugin execution will be skipped ({} : reconciled in {} millis)", elapsed, fullGoalName);
                 }
+
+                LOGGER.debug(
+                        "Checked {}, resolved mojo: {}, cached params: {}", fullGoalName, mojo, completedExecution);
+
             } catch (PluginContainerException | PluginConfigurationException e) {
                 throw new LifecycleExecutionException("Cannot get configured mojo", e);
             } finally {
@@ -278,8 +321,8 @@ private boolean verifyCacheConsistency(
                 }
             }
         } else {
-            LOGGER.info(
-                    "Skipping plugin execution (cached): {}",
+            LOGGER.debug(
+                    "Plugin execution will be skipped ({} : cached)",
                     cacheCandidate.getMojoDescriptor().getFullGoalName());
         }
 
@@ -364,4 +407,10 @@ private static String normalizedPath(Path path, Path baseDirPath) {
         }
         return normalizedPath;
     }
+
+    private enum CacheRestorationStatus {
+        SUCCESS,
+        FAILURE,
+        FAILURE_NEEDS_CLEAN;
+    }
 }

src/main/java/org/apache/maven/buildcache/CacheController.java

@@ -21,6 +21,7 @@
 import java.util.List;
 import java.util.Map;
 
+import org.apache.maven.buildcache.artifact.ArtifactRestorationReport;
 import org.apache.maven.execution.MavenSession;
 import org.apache.maven.execution.MojoExecutionEvent;
 import org.apache.maven.plugin.MojoExecution;
@@ -34,7 +35,7 @@ public interface CacheController {
     CacheResult findCachedBuild(
             MavenSession session, MavenProject project, List<MojoExecution> mojoExecutions, boolean skipCache);
 
-    boolean restoreProjectArtifacts(CacheResult cacheResult);
+    ArtifactRestorationReport restoreProjectArtifacts(CacheResult cacheResult);
 
     void save(
             CacheResult cacheResult,

src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java

@@ -46,6 +46,7 @@
 import java.util.UUID;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.concurrent.FutureTask;
 import java.util.regex.Pattern;
@@ -55,8 +56,10 @@
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.mutable.MutableBoolean;
 import org.apache.maven.SessionScoped;
+import org.apache.maven.artifact.InvalidArtifactRTException;
 import org.apache.maven.artifact.handler.ArtifactHandler;
 import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager;
+import org.apache.maven.buildcache.artifact.ArtifactRestorationReport;
 import org.apache.maven.buildcache.artifact.RestoredArtifact;
 import org.apache.maven.buildcache.checksum.MavenProjectInput;
 import org.apache.maven.buildcache.hash.HashAlgorithm;
@@ -295,10 +298,13 @@ private boolean canIgnoreMissingSegment(MavenProject project, Build info, List<M
     }
 
     @Override
-    public boolean restoreProjectArtifacts(CacheResult cacheResult) {
+    public ArtifactRestorationReport restoreProjectArtifacts(CacheResult cacheResult) {
+
+        LOGGER.debug("Restore project artifacts");
         final Build build = cacheResult.getBuildInfo();
         final CacheContext context = cacheResult.getContext();
         final MavenProject project = context.getProject();
+        ArtifactRestorationReport restorationReport = new ArtifactRestorationReport();
 
         try {
             RestoredArtifact restoredProjectArtifact = null;
@@ -325,6 +331,8 @@ public boolean restoreProjectArtifacts(CacheResult cacheResult) {
                         // it may also be disabled on a per-project level (defaults to true - enable)
                         if (cacheConfig.isRestoreGeneratedSources()
                                 && MavenProjectInput.isRestoreGeneratedSources(project)) {
+                            // Set this value before trying the restoration, to keep a trace of the attempt if it fails
+                            restorationReport.setRestoredFilesInProjectDirectory(true);
                             // generated sources artifact
                             final Path attachedArtifactFile =
                                     localCache.getArtifactFile(context, cacheResult.getSource(), attachedArtifactInfo);
@@ -348,11 +356,11 @@ public boolean restoreProjectArtifacts(CacheResult cacheResult) {
                 project.setArtifact(restoredProjectArtifact);
             }
             restoredAttachedArtifacts.forEach(project::addAttachedArtifact);
-            return true;
+            restorationReport.setSuccess(true);
         } catch (Exception e) {
             LOGGER.debug("Cannot restore cache, continuing with normal build.", e);
-            return false;
         }
+        return restorationReport;
     }
 
     /**
@@ -401,6 +409,26 @@ private Future<File> createDownloadTask(
         });
         if (!cacheConfig.isLazyRestore()) {
             downloadTask.run();
+            try {
+                downloadTask.get();
+            } catch (InterruptedException e) {
+                Thread.currentThread().interrupt();
+                throw new InvalidArtifactRTException(
+                        artifact.getGroupId(),
+                        artifact.getArtifactId(),
+                        artifact.getVersion(),
+                        artifact.getType(),
+                        RestoredArtifact.MSG_INTERRUPTED_WHILE_RETRIEVING_ARTIFACT_FILE,
+                        e);
+            } catch (ExecutionException e) {
+                throw new InvalidArtifactRTException(
+                        artifact.getGroupId(),
+                        artifact.getArtifactId(),
+                        artifact.getVersion(),
+                        artifact.getType(),
+                        RestoredArtifact.MSG_ERROR_RETRIEVING_ARTIFACT_FILE,
+                        e.getCause());
+            }
         }
         return downloadTask;
     }

src/main/java/org/apache/maven/buildcache/MojoParametersListener.java

@@ -77,8 +77,4 @@ public void afterExecutionFailure(MojoExecutionEvent event) {
     public Map<String, MojoExecutionEvent> getProjectExecutions(MavenProject project) {
         return projectExecutions.get(project);
     }
-
-    public void remove(MavenProject project) {
-        projectExecutions.remove(project);
-    }
 }

src/main/java/org/apache/maven/buildcache/artifact/ArtifactRestorationReport.java

@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.buildcache.artifact;
+
+public class ArtifactRestorationReport {
+
+    /**
+     * Success restoration indicator
+     */
+    private boolean success;
+
+    /**
+     * True if some files have been restored (or attempted in case of error) in the project directory
+     */
+    private boolean restoredFilesInProjectDirectory;
+
+    public boolean isSuccess() {
+        return success;
+    }
+
+    public void setSuccess(boolean success) {
+        this.success = success;
+    }
+
+    public boolean isRestoredFilesInProjectDirectory() {
+        return restoredFilesInProjectDirectory;
+    }
+
+    public void setRestoredFilesInProjectDirectory(boolean restoredFilesInProjectDirectory) {
+        this.restoredFilesInProjectDirectory = restoredFilesInProjectDirectory;
+    }
+}

src/main/java/org/apache/maven/buildcache/artifact/RestoredArtifact.java

@@ -40,6 +40,9 @@
 public class RestoredArtifact extends DefaultArtifact {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(RestoredArtifact.class);
+    public static final String MSG_INTERRUPTED_WHILE_RETRIEVING_ARTIFACT_FILE =
+            "Interrupted while retrieving artifact file";
+    public static final String MSG_ERROR_RETRIEVING_ARTIFACT_FILE = "Error retrieving artifact file";
 
     private volatile Future<File> fileFuture;
 
@@ -75,7 +78,7 @@ public File getFile() {
                             getArtifactId(),
                             getVersion(),
                             getType(),
-                            "Error retrieving artifact file",
+                            MSG_ERROR_RETRIEVING_ARTIFACT_FILE,
                             e);
                 }
             } else {
@@ -94,15 +97,15 @@ public File getFile() {
                     getArtifactId(),
                     getVersion(),
                     getType(),
-                    "Interrupted while retrieving artifact file",
+                    MSG_INTERRUPTED_WHILE_RETRIEVING_ARTIFACT_FILE,
                     e);
         } catch (ExecutionException e) {
             throw new InvalidArtifactRTException(
                     getGroupId(),
                     getArtifactId(),
                     getVersion(),
                     getType(),
-                    "Error retrieving artifact file",
+                    MSG_ERROR_RETRIEVING_ARTIFACT_FILE,
                     e.getCause());
         }
     }