From 19f4a0a8307a8b0f2f672c3f8544d5ef26205693 Mon Sep 17 00:00:00 2001 From: cowwoc Date: Tue, 7 Oct 2025 23:51:45 -0400 Subject: [PATCH 1/6] Save attached outputs for compile-only cache entries --- .../maven/buildcache/CacheControllerImpl.java | 61 ++++++++---------- .../its/Issue393CompileRestoreTest.java | 51 +++++++++++++++ .../.mvn/extensions.xml | 28 ++++++++ .../.mvn/maven-build-cache-config.xml | 34 ++++++++++ .../issue-393-compile-restore/app/pom.xml | 37 +++++++++++ .../app/src/main/java/module-info.java | 3 + .../maven/caching/test/jpms/app/Greeting.java | 12 ++++ .../consumer/pom.xml | 64 +++++++++++++++++++ .../consumer/src/main/java/module-info.java | 4 ++ .../caching/test/jpms/consumer/Consumer.java | 14 ++++ .../test/jpms/consumer/ConsumerTest.java | 13 ++++ .../issue-393-compile-restore/pom.xml | 42 ++++++++++++ 12 files changed, 330 insertions(+), 33 deletions(-) create mode 100644 src/test/java/org/apache/maven/buildcache/its/Issue393CompileRestoreTest.java create mode 100644 src/test/projects/issue-393-compile-restore/.mvn/extensions.xml create mode 100644 src/test/projects/issue-393-compile-restore/.mvn/maven-build-cache-config.xml create mode 100644 src/test/projects/issue-393-compile-restore/app/pom.xml create mode 100644 src/test/projects/issue-393-compile-restore/app/src/main/java/module-info.java create mode 100644 src/test/projects/issue-393-compile-restore/app/src/main/java/org/apache/maven/caching/test/jpms/app/Greeting.java create mode 100644 src/test/projects/issue-393-compile-restore/consumer/pom.xml create mode 100644 src/test/projects/issue-393-compile-restore/consumer/src/main/java/module-info.java create mode 100644 src/test/projects/issue-393-compile-restore/consumer/src/main/java/org/apache/maven/caching/test/jpms/consumer/Consumer.java create mode 100644 src/test/projects/issue-393-compile-restore/consumer/src/test/java/org/apache/maven/caching/test/jpms/consumer/ConsumerTest.java create mode 100644 src/test/projects/issue-393-compile-restore/pom.xml diff --git a/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java b/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java index e71dcb7a..3134d9b8 100644 --- a/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java +++ b/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java @@ -498,25 +498,23 @@ public void save( final MavenProject project = context.getProject(); final MavenSession session = context.getSession(); try { + attachedResourcesPathsById.clear(); + attachedResourceCounter = 0; + final HashFactory hashFactory = cacheConfig.getHashFactory(); + final HashAlgorithm algorithm = hashFactory.createAlgorithm(); final org.apache.maven.artifact.Artifact projectArtifact = project.getArtifact(); - final List attachedArtifacts; - final List attachedArtifactDtos; - final Artifact projectArtifactDto; - if (project.hasLifecyclePhase("package")) { - final HashAlgorithm algorithm = hashFactory.createAlgorithm(); - attachGeneratedSources(project); - attachOutputs(project); - attachedArtifacts = project.getAttachedArtifacts() != null - ? project.getAttachedArtifacts() - : Collections.emptyList(); - attachedArtifactDtos = artifactDtos(attachedArtifacts, algorithm, project); - projectArtifactDto = artifactDto(project.getArtifact(), algorithm, project); - } else { - attachedArtifacts = Collections.emptyList(); - attachedArtifactDtos = new ArrayList<>(); - projectArtifactDto = null; - } + final boolean hasPackagePhase = project.hasLifecyclePhase("package"); + + attachGeneratedSources(project); + attachOutputs(project); + + final List attachedArtifacts = project.getAttachedArtifacts() != null + ? project.getAttachedArtifacts() + : Collections.emptyList(); + final List attachedArtifactDtos = artifactDtos(attachedArtifacts, algorithm, project); + final Artifact projectArtifactDto = hasPackagePhase ? artifactDto(project.getArtifact(), algorithm, project) + : null; List completedExecution = buildExecutionInfo(mojoExecutions, executionEvents); @@ -534,22 +532,19 @@ public void save( localCache.beforeSave(context); // if package phase presence means new artifacts were packaged - if (project.hasLifecyclePhase("package")) { - if (projectArtifact.getFile() != null) { - localCache.saveArtifactFile(cacheResult, projectArtifact); - } - for (org.apache.maven.artifact.Artifact attachedArtifact : attachedArtifacts) { - if (attachedArtifact.getFile() != null) { - boolean storeArtifact = - isOutputArtifact(attachedArtifact.getFile().getName()); - if (storeArtifact) { - localCache.saveArtifactFile(cacheResult, attachedArtifact); - } else { - LOGGER.debug( - "Skipping attached project artifact '{}' = " - + " it is marked for exclusion from caching", - attachedArtifact.getFile().getName()); - } + if (hasPackagePhase && projectArtifact.getFile() != null) { + localCache.saveArtifactFile(cacheResult, projectArtifact); + } + for (org.apache.maven.artifact.Artifact attachedArtifact : attachedArtifacts) { + if (attachedArtifact.getFile() != null) { + boolean storeArtifact = isOutputArtifact(attachedArtifact.getFile().getName()); + if (storeArtifact) { + localCache.saveArtifactFile(cacheResult, attachedArtifact); + } else { + LOGGER.debug( + "Skipping attached project artifact '{}' = " + + " it is marked for exclusion from caching", + attachedArtifact.getFile().getName()); } } } diff --git a/src/test/java/org/apache/maven/buildcache/its/Issue393CompileRestoreTest.java b/src/test/java/org/apache/maven/buildcache/its/Issue393CompileRestoreTest.java new file mode 100644 index 00000000..26089fd9 --- /dev/null +++ b/src/test/java/org/apache/maven/buildcache/its/Issue393CompileRestoreTest.java @@ -0,0 +1,51 @@ +/* + * 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.its; + +import java.util.Arrays; + +import org.apache.maven.buildcache.its.junit.IntegrationTest; +import org.apache.maven.it.VerificationException; +import org.apache.maven.it.Verifier; +import org.junit.jupiter.api.Test; + +@IntegrationTest("src/test/projects/issue-393-compile-restore") +class Issue393CompileRestoreTest { + + @Test + void restoresAttachedOutputsAfterCompileOnlyBuild(Verifier verifier) throws VerificationException { + verifier.setAutoclean(false); + + verifier.setLogFileName("../log-compile.txt"); + verifier.executeGoals(Arrays.asList("clean", "compile")); + verifier.verifyErrorFreeLog(); + verifier.verifyFilePresent("app/target/classes/module-info.class"); + verifier.verifyFilePresent("consumer/target/classes/module-info.class"); + + verifier.setLogFileName("../log-verify.txt"); + verifier.executeGoals(Arrays.asList("clean", "verify")); + verifier.verifyErrorFreeLog(); + verifier.verifyTextInLog("Found cached build, restoring org.apache.maven.caching.test.jpms:issue-393-app from cache"); + verifier.verifyTextInLog("Skipping plugin execution (cached): compiler:compile"); + + verifier.verifyFilePresent("app/target/classes/module-info.class"); + verifier.verifyFilePresent("consumer/target/classes/module-info.class"); + verifier.verifyFilePresent("consumer/target/test-classes/org/apache/maven/caching/test/jpms/consumer/ConsumerTest.class"); + } +} diff --git a/src/test/projects/issue-393-compile-restore/.mvn/extensions.xml b/src/test/projects/issue-393-compile-restore/.mvn/extensions.xml new file mode 100644 index 00000000..8568c4d0 --- /dev/null +++ b/src/test/projects/issue-393-compile-restore/.mvn/extensions.xml @@ -0,0 +1,28 @@ + + + + + org.apache.maven.extensions + maven-build-cache-extension + ${projectVersion} + + diff --git a/src/test/projects/issue-393-compile-restore/.mvn/maven-build-cache-config.xml b/src/test/projects/issue-393-compile-restore/.mvn/maven-build-cache-config.xml new file mode 100644 index 00000000..6a91d577 --- /dev/null +++ b/src/test/projects/issue-393-compile-restore/.mvn/maven-build-cache-config.xml @@ -0,0 +1,34 @@ + + + + + + + + + classes + test-classes + maven-status + + + + diff --git a/src/test/projects/issue-393-compile-restore/app/pom.xml b/src/test/projects/issue-393-compile-restore/app/pom.xml new file mode 100644 index 00000000..f732e706 --- /dev/null +++ b/src/test/projects/issue-393-compile-restore/app/pom.xml @@ -0,0 +1,37 @@ + + + + 4.0.0 + + + org.apache.maven.caching.test.jpms + issue-393-compile-restore + 0.0.1-SNAPSHOT + + + issue-393-app + Issue 393 Compile Restore - App Module + jar + + diff --git a/src/test/projects/issue-393-compile-restore/app/src/main/java/module-info.java b/src/test/projects/issue-393-compile-restore/app/src/main/java/module-info.java new file mode 100644 index 00000000..ce82afdc --- /dev/null +++ b/src/test/projects/issue-393-compile-restore/app/src/main/java/module-info.java @@ -0,0 +1,3 @@ +module org.apache.maven.caching.test.jpms.app { + exports org.apache.maven.caching.test.jpms.app; +} diff --git a/src/test/projects/issue-393-compile-restore/app/src/main/java/org/apache/maven/caching/test/jpms/app/Greeting.java b/src/test/projects/issue-393-compile-restore/app/src/main/java/org/apache/maven/caching/test/jpms/app/Greeting.java new file mode 100644 index 00000000..44483f7b --- /dev/null +++ b/src/test/projects/issue-393-compile-restore/app/src/main/java/org/apache/maven/caching/test/jpms/app/Greeting.java @@ -0,0 +1,12 @@ +package org.apache.maven.caching.test.jpms.app; + +public final class Greeting { + + private Greeting() { + // utility + } + + public static String message() { + return "hello from module"; + } +} diff --git a/src/test/projects/issue-393-compile-restore/consumer/pom.xml b/src/test/projects/issue-393-compile-restore/consumer/pom.xml new file mode 100644 index 00000000..ab7a00ee --- /dev/null +++ b/src/test/projects/issue-393-compile-restore/consumer/pom.xml @@ -0,0 +1,64 @@ + + + + 4.0.0 + + + org.apache.maven.caching.test.jpms + issue-393-compile-restore + 0.0.1-SNAPSHOT + + + issue-393-consumer + Issue 393 Compile Restore - Consumer Module + jar + + + + ${project.groupId} + issue-393-app + ${project.version} + + + org.junit.jupiter + junit-jupiter + 5.10.2 + test + + + + + + + org.apache.maven.plugins + maven-surefire-plugin + 3.2.5 + + true + + + + + + diff --git a/src/test/projects/issue-393-compile-restore/consumer/src/main/java/module-info.java b/src/test/projects/issue-393-compile-restore/consumer/src/main/java/module-info.java new file mode 100644 index 00000000..52d0dc94 --- /dev/null +++ b/src/test/projects/issue-393-compile-restore/consumer/src/main/java/module-info.java @@ -0,0 +1,4 @@ +module org.apache.maven.caching.test.jpms.consumer { + requires org.apache.maven.caching.test.jpms.app; + exports org.apache.maven.caching.test.jpms.consumer; +} diff --git a/src/test/projects/issue-393-compile-restore/consumer/src/main/java/org/apache/maven/caching/test/jpms/consumer/Consumer.java b/src/test/projects/issue-393-compile-restore/consumer/src/main/java/org/apache/maven/caching/test/jpms/consumer/Consumer.java new file mode 100644 index 00000000..a64ad993 --- /dev/null +++ b/src/test/projects/issue-393-compile-restore/consumer/src/main/java/org/apache/maven/caching/test/jpms/consumer/Consumer.java @@ -0,0 +1,14 @@ +package org.apache.maven.caching.test.jpms.consumer; + +import org.apache.maven.caching.test.jpms.app.Greeting; + +public final class Consumer { + + private Consumer() { + // utility + } + + public static String message() { + return Greeting.message(); + } +} diff --git a/src/test/projects/issue-393-compile-restore/consumer/src/test/java/org/apache/maven/caching/test/jpms/consumer/ConsumerTest.java b/src/test/projects/issue-393-compile-restore/consumer/src/test/java/org/apache/maven/caching/test/jpms/consumer/ConsumerTest.java new file mode 100644 index 00000000..292a7a52 --- /dev/null +++ b/src/test/projects/issue-393-compile-restore/consumer/src/test/java/org/apache/maven/caching/test/jpms/consumer/ConsumerTest.java @@ -0,0 +1,13 @@ +package org.apache.maven.caching.test.jpms.consumer; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class ConsumerTest { + + @Test + void messageIsProvidedByUpstreamModule() { + assertEquals("hello from module", Consumer.message()); + } +} diff --git a/src/test/projects/issue-393-compile-restore/pom.xml b/src/test/projects/issue-393-compile-restore/pom.xml new file mode 100644 index 00000000..f504867a --- /dev/null +++ b/src/test/projects/issue-393-compile-restore/pom.xml @@ -0,0 +1,42 @@ + + + + 4.0.0 + org.apache.maven.caching.test.jpms + issue-393-compile-restore + 0.0.1-SNAPSHOT + pom + Issue 393 Compile Restore Aggregator + + + app + consumer + + + + UTF-8 + 17 + + + From 92822da7168d9feae5e577f05e1dfb4f1fa8ebad Mon Sep 17 00:00:00 2001 From: cowwoc Date: Sat, 11 Oct 2025 23:37:53 -0400 Subject: [PATCH 2/6] Add timestamp filtering and per-project thread-safe isolation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit addresses reviewer concerns about: 1. Branch-switching scenario with stale artifacts 2. Thread safety in multi-threaded builds (mvn -T) Changes: **Timestamp Filtering:** - Capture build start time from session.getRequest().getStartTime() - Pass buildStartTime to attachGeneratedSources() and attachOutputs() - Check directory last-modified time in attachDirIfNotEmpty() - Skip directories with lastModified < buildStartTime (unless restored) **Per-Project Thread-Safe Isolation:** - Introduce ProjectCacheState inner class to hold per-project state: * attachedResourcesPathsById (resource path tracking) * attachedResourceCounter (unique classifier generation) * restoredOutputClassifiers (track restored outputs) - Use ConcurrentHashMap for thread-safe access - Track restored outputs in restoreProjectArtifacts() - Include restored outputs even with old timestamps - Cleanup project state after save() completes **Thread Safety Benefits:** - Each project gets isolated state (no cross-module interference) - ConcurrentHashMap handles concurrent module builds - Per-project cleanup prevents memory leaks - Fixes existing thread safety bug with HashMap.clear() **How Timestamp Filtering Works:** ``` Scenario: mvn package with compile cache hit 1. First build: mvn compile at 10:00:00 - Compiles, caches outputs ✓ 2. Second build: mvn package at 10:05:00 - Compile phase: Restores from cache - Restoration tracked in state.restoredOutputClassifiers - save() phase: Checks timestamps * Fresh outputs: timestamp >= 10:05:00 → include ✓ * Restored outputs: in restoredOutputClassifiers → include ✓ * Stale outputs: timestamp < 10:05:00 AND not restored → exclude ✓ ``` This prevents the branch-switching scenario: ``` 1. mvn compile on Branch A → target/classes modified at 10:00:01 2. git checkout branch-b 3. mvn process-resources on Branch B starts at 10:01:00 4. Check target/classes: 10:00:01 < 10:01:00 AND not restored → SKIP ✓ ``` --- .../maven/buildcache/CacheControllerImpl.java | 91 ++++++++++++++----- 1 file changed, 66 insertions(+), 25 deletions(-) diff --git a/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java b/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java index 3134d9b8..31413089 100644 --- a/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java +++ b/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java @@ -40,6 +40,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -135,13 +136,24 @@ public class CacheControllerImpl implements CacheController { private volatile Scm scm; /** - * A map dedicated to store the base path of resources stored to the cache which are not original artifacts - * (ex : generated source basedir). - * Used to link the resource to its path on disk + * Per-project cache state to ensure thread safety in multi-threaded builds. + * Each project gets isolated state for resource tracking, counters, and restored output tracking. */ - private final Map attachedResourcesPathsById = new HashMap<>(); + private static class ProjectCacheState { + final Map attachedResourcesPathsById = new HashMap<>(); + int attachedResourceCounter = 0; + final Set restoredOutputClassifiers = new HashSet<>(); + } + + private final ConcurrentMap projectStates = new ConcurrentHashMap<>(); - private int attachedResourceCounter = 0; + /** + * Get or create cache state for the given project (thread-safe). + */ + private ProjectCacheState getProjectState(MavenProject project) { + String key = getVersionlessProjectKey(project); + return projectStates.computeIfAbsent(key, k -> new ProjectCacheState()); + } // CHECKSTYLE_OFF: ParameterNumber @Inject public CacheControllerImpl( @@ -356,6 +368,7 @@ public ArtifactRestorationReport restoreProjectArtifacts(CacheResult cacheResult final Build build = cacheResult.getBuildInfo(); final CacheContext context = cacheResult.getContext(); final MavenProject project = context.getProject(); + final ProjectCacheState state = getProjectState(project); ArtifactRestorationReport restorationReport = new ArtifactRestorationReport(); try { @@ -397,6 +410,8 @@ public ArtifactRestorationReport restoreProjectArtifacts(CacheResult cacheResult final Path attachedArtifactFile = localCache.getArtifactFile(context, cacheResult.getSource(), attachedArtifactInfo); restoreGeneratedSources(attachedArtifactInfo, attachedArtifactFile, project); + // Track this classifier as restored so save() includes it even with old timestamp + state.restoredOutputClassifiers.add(attachedArtifactInfo.getClassifier()); } } else { Future downloadTask = createDownloadTask( @@ -497,23 +512,27 @@ public void save( final MavenProject project = context.getProject(); final MavenSession session = context.getSession(); + final ProjectCacheState state = getProjectState(project); try { - attachedResourcesPathsById.clear(); - attachedResourceCounter = 0; + state.attachedResourcesPathsById.clear(); + state.attachedResourceCounter = 0; + + // Get build start time to filter out stale artifacts from previous builds + final long buildStartTime = session.getRequest().getStartTime().getTime(); final HashFactory hashFactory = cacheConfig.getHashFactory(); final HashAlgorithm algorithm = hashFactory.createAlgorithm(); final org.apache.maven.artifact.Artifact projectArtifact = project.getArtifact(); final boolean hasPackagePhase = project.hasLifecyclePhase("package"); - attachGeneratedSources(project); - attachOutputs(project); + attachGeneratedSources(project, state, buildStartTime); + attachOutputs(project, state, buildStartTime); final List attachedArtifacts = project.getAttachedArtifacts() != null ? project.getAttachedArtifacts() : Collections.emptyList(); - final List attachedArtifactDtos = artifactDtos(attachedArtifacts, algorithm, project); - final Artifact projectArtifactDto = hasPackagePhase ? artifactDto(project.getArtifact(), algorithm, project) + final List attachedArtifactDtos = artifactDtos(attachedArtifacts, algorithm, project, state); + final Artifact projectArtifactDto = hasPackagePhase ? artifactDto(project.getArtifact(), algorithm, project, state) : null; List completedExecution = buildExecutionInfo(mojoExecutions, executionEvents); @@ -562,6 +581,10 @@ public void save( } catch (Exception ex) { LOGGER.error("Failed to clean cache due to unexpected error:", ex); } + } finally { + // Cleanup project state to free memory (thread-safe removal) + String key = getVersionlessProjectKey(project); + projectStates.remove(key); } } @@ -619,20 +642,22 @@ public void produceDiffReport(CacheResult cacheResult, Build build) { } private List artifactDtos( - List attachedArtifacts, HashAlgorithm digest, MavenProject project) + List attachedArtifacts, HashAlgorithm digest, MavenProject project, + ProjectCacheState state) throws IOException { List result = new ArrayList<>(); for (org.apache.maven.artifact.Artifact attachedArtifact : attachedArtifacts) { if (attachedArtifact.getFile() != null && isOutputArtifact(attachedArtifact.getFile().getName())) { - result.add(artifactDto(attachedArtifact, digest, project)); + result.add(artifactDto(attachedArtifact, digest, project, state)); } } return result; } private Artifact artifactDto( - org.apache.maven.artifact.Artifact projectArtifact, HashAlgorithm algorithm, MavenProject project) + org.apache.maven.artifact.Artifact projectArtifact, HashAlgorithm algorithm, MavenProject project, + ProjectCacheState state) throws IOException { final Artifact dto = DtoUtils.createDto(projectArtifact); if (projectArtifact.getFile() != null && projectArtifact.getFile().isFile()) { @@ -641,7 +666,7 @@ private Artifact artifactDto( dto.setFileSize(Files.size(file)); // Get the relative path of any extra zip directory added to the cache - Path relativePath = attachedResourcesPathsById.get(projectArtifact.getClassifier()); + Path relativePath = state.attachedResourcesPathsById.get(projectArtifact.getClassifier()); if (relativePath == null) { // If the path was not a member of this map, we are in presence of an original artifact. // we get its location on the disk @@ -895,15 +920,15 @@ private void restoreGeneratedSources(Artifact artifact, Path artifactFilePath, M } // TODO: move to config - public void attachGeneratedSources(MavenProject project) throws IOException { + public void attachGeneratedSources(MavenProject project, ProjectCacheState state, long buildStartTime) throws IOException { final Path targetDir = Paths.get(project.getBuild().getDirectory()); final Path generatedSourcesDir = targetDir.resolve("generated-sources"); - attachDirIfNotEmpty(generatedSourcesDir, targetDir, project, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB); + attachDirIfNotEmpty(generatedSourcesDir, targetDir, project, state, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB, buildStartTime); final Path generatedTestSourcesDir = targetDir.resolve("generated-test-sources"); attachDirIfNotEmpty( - generatedTestSourcesDir, targetDir, project, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB); + generatedTestSourcesDir, targetDir, project, state, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB, buildStartTime); Set sourceRoots = new TreeSet<>(); if (project.getCompileSourceRoots() != null) { @@ -919,18 +944,18 @@ public void attachGeneratedSources(MavenProject project) throws IOException { && sourceRootPath.startsWith(targetDir) && !(sourceRootPath.startsWith(generatedSourcesDir) || sourceRootPath.startsWith(generatedTestSourcesDir))) { // dir within target - attachDirIfNotEmpty(sourceRootPath, targetDir, project, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB); + attachDirIfNotEmpty(sourceRootPath, targetDir, project, state, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB, buildStartTime); } } } - private void attachOutputs(MavenProject project) throws IOException { + private void attachOutputs(MavenProject project, ProjectCacheState state, long buildStartTime) throws IOException { final List attachedDirs = cacheConfig.getAttachedOutputs(); for (DirName dir : attachedDirs) { final Path targetDir = Paths.get(project.getBuild().getDirectory()); final Path outputDir = targetDir.resolve(dir.getValue()); if (isPathInsideProject(project, outputDir)) { - attachDirIfNotEmpty(outputDir, targetDir, project, OutputType.EXTRA_OUTPUT, dir.getGlob()); + attachDirIfNotEmpty(outputDir, targetDir, project, state, OutputType.EXTRA_OUTPUT, dir.getGlob(), buildStartTime); } else { LOGGER.warn("Outside project output candidate directory discarded ({})", outputDir.normalize()); } @@ -941,16 +966,32 @@ private void attachDirIfNotEmpty( Path candidateSubDir, Path parentDir, MavenProject project, + ProjectCacheState state, final OutputType attachedOutputType, - final String glob) + final String glob, + final long buildStartTime) throws IOException { if (Files.isDirectory(candidateSubDir) && hasFiles(candidateSubDir)) { final Path relativePath = project.getBasedir().toPath().relativize(candidateSubDir); - attachedResourceCounter++; - final String classifier = attachedOutputType.getClassifierPrefix() + attachedResourceCounter; + state.attachedResourceCounter++; + final String classifier = attachedOutputType.getClassifierPrefix() + state.attachedResourceCounter; + + // Check if directory was modified during this build OR was restored from cache + long lastModified = Files.getLastModifiedTime(candidateSubDir).toMillis(); + boolean isRestoredThisBuild = state.restoredOutputClassifiers.contains(classifier); + + if (lastModified < buildStartTime && !isRestoredThisBuild) { + LOGGER.debug( + "Skipping stale directory: {} (modified at {}, build started at {}, not restored)", + candidateSubDir, + lastModified, + buildStartTime); + return; + } + boolean success = zipAndAttachArtifact(project, candidateSubDir, classifier, glob); if (success) { - attachedResourcesPathsById.put(classifier, relativePath); + state.attachedResourcesPathsById.put(classifier, relativePath); LOGGER.debug("Attached directory: {}", candidateSubDir); } } From 3126fe127fbcf890157fc5fb708ffdc479908b7b Mon Sep 17 00:00:00 2001 From: cowwoc Date: Sun, 12 Oct 2025 01:42:35 -0400 Subject: [PATCH 3/6] Add maven.build.cache.cacheCompile property to control compile-phase caching Added new configuration property maven.build.cache.cacheCompile (default: true) to address reviewer concerns about performance overhead during active development. Changes: - CacheConfig: Added isCacheCompile() method with javadoc - CacheConfigImpl: Added CACHE_COMPILE constant and implementation (default true) - CacheControllerImpl: Wrapped attachGeneratedSources() and attachOutputs() calls in isCacheCompile() check - CacheCompileDisabledTest: Integration tests verifying behavior when disabled Benefits: - Maintains fix for issue #393 (default behavior unchanged) - Allows developers to opt-out of compile caching with: -Dmaven.build.cache.cacheCompile=false - Reduces IO overhead (no zipping/uploading) during active development - Users working on large multi-module projects can keep it enabled Usage: # Disable compile caching to reduce overhead during development: mvn clean compile -Dmaven.build.cache.cacheCompile=false # Enable for CI/multi-module scenarios (default): mvn clean compile --- .../maven/buildcache/CacheControllerImpl.java | 24 ++- .../maven/buildcache/xml/CacheConfig.java | 11 ++ .../maven/buildcache/xml/CacheConfigImpl.java | 6 + .../its/CacheCompileDisabledTest.java | 137 ++++++++++++++++++ 4 files changed, 176 insertions(+), 2 deletions(-) create mode 100644 src/test/java/org/apache/maven/buildcache/its/CacheCompileDisabledTest.java diff --git a/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java b/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java index 31413089..2934cab9 100644 --- a/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java +++ b/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java @@ -525,8 +525,14 @@ public void save( final org.apache.maven.artifact.Artifact projectArtifact = project.getArtifact(); final boolean hasPackagePhase = project.hasLifecyclePhase("package"); - attachGeneratedSources(project, state, buildStartTime); - attachOutputs(project, state, buildStartTime); + // Cache compile outputs (classes, test-classes, generated sources) if enabled + // This allows compile-only builds to create restorable cache entries + // Can be disabled with -Dmaven.build.cache.cacheCompile=false to reduce IO overhead + final boolean cacheCompile = cacheConfig.isCacheCompile(); + if (cacheCompile) { + attachGeneratedSources(project, state, buildStartTime); + attachOutputs(project, state, buildStartTime); + } final List attachedArtifacts = project.getAttachedArtifacts() != null ? project.getAttachedArtifacts() @@ -535,6 +541,20 @@ public void save( final Artifact projectArtifactDto = hasPackagePhase ? artifactDto(project.getArtifact(), algorithm, project, state) : null; + // CRITICAL: Don't create incomplete cache entries! + // Only save cache entry if we have SOMETHING useful to restore. + // Exclude consumer POMs (Maven metadata) from the "useful artifacts" check. + // This prevents the bug where: + // 1. mvn compile (cacheCompile=false) creates cache entry with only metadata + // 2. mvn compile (cacheCompile=true) tries to restore incomplete cache and fails + boolean hasUsefulArtifacts = projectArtifactDto != null + || attachedArtifactDtos.stream() + .anyMatch(a -> !"consumer".equals(a.getClassifier()) || !"pom".equals(a.getType())); + if (!hasUsefulArtifacts) { + LOGGER.info("Skipping cache save: no artifacts to save (only metadata present)"); + return; + } + List completedExecution = buildExecutionInfo(mojoExecutions, executionEvents); final Build build = new Build( diff --git a/src/main/java/org/apache/maven/buildcache/xml/CacheConfig.java b/src/main/java/org/apache/maven/buildcache/xml/CacheConfig.java index d86b15d4..56d33147 100644 --- a/src/main/java/org/apache/maven/buildcache/xml/CacheConfig.java +++ b/src/main/java/org/apache/maven/buildcache/xml/CacheConfig.java @@ -152,4 +152,15 @@ public interface CacheConfig { * Flag to save in cache only if a build went through the clean lifecycle */ boolean isMandatoryClean(); + + /** + * Flag to cache compile phase outputs (classes, test-classes, generated sources). + * When enabled (default), compile-only builds create cache entries that can be restored + * by subsequent builds. When disabled, caching only occurs during package phase or later. + *

+ * Use: -Dmaven.build.cache.cacheCompile=(true|false) + *

+ * Default: true + */ + boolean isCacheCompile(); } diff --git a/src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java b/src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java index cd6e87c0..9a6dfd4c 100644 --- a/src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java +++ b/src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java @@ -97,6 +97,7 @@ public class CacheConfigImpl implements org.apache.maven.buildcache.xml.CacheCon public static final String RESTORE_GENERATED_SOURCES_PROPERTY_NAME = "maven.build.cache.restoreGeneratedSources"; public static final String ALWAYS_RUN_PLUGINS = "maven.build.cache.alwaysRunPlugins"; public static final String MANDATORY_CLEAN = "maven.build.cache.mandatoryClean"; + public static final String CACHE_COMPILE = "maven.build.cache.cacheCompile"; /** * Flag to control if we should skip lookup for cached artifacts globally or for a particular project even if @@ -541,6 +542,11 @@ public boolean isMandatoryClean() { return getProperty(MANDATORY_CLEAN, getConfiguration().isMandatoryClean()); } + @Override + public boolean isCacheCompile() { + return getProperty(CACHE_COMPILE, true); + } + @Override public String getId() { checkInitializedState(); diff --git a/src/test/java/org/apache/maven/buildcache/its/CacheCompileDisabledTest.java b/src/test/java/org/apache/maven/buildcache/its/CacheCompileDisabledTest.java new file mode 100644 index 00000000..47e7dcbc --- /dev/null +++ b/src/test/java/org/apache/maven/buildcache/its/CacheCompileDisabledTest.java @@ -0,0 +1,137 @@ +/* + * 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.its; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.stream.Stream; + +import org.apache.maven.buildcache.its.junit.IntegrationTest; +import org.apache.maven.it.VerificationException; +import org.apache.maven.it.Verifier; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Tests that the maven.build.cache.cacheCompile property correctly disables + * caching of compile-phase outputs. + */ +@IntegrationTest("src/test/projects/issue-393-compile-restore") +class CacheCompileDisabledTest { + + @Test + void compileDoesNotCacheWhenDisabled(Verifier verifier) throws VerificationException, IOException { + verifier.setAutoclean(false); + + // The actual cache is stored in target/build-cache (relative to the extension root, not test project) + Path localCache = Paths.get(System.getProperty("maven.multiModuleProjectDirectory")) + .resolve("target/build-cache"); + + // Clean cache before test + if (Files.exists(localCache)) { + deleteDirectory(localCache); + } + + // First compile with cacheCompile disabled - compile only the app module to avoid dependency issues + verifier.setLogFileName("../log-compile-disabled.txt"); + verifier.addCliOption("-Dmaven.build.cache.cacheCompile=false"); + verifier.addCliOption("-pl"); + verifier.addCliOption("app"); + verifier.executeGoals(Arrays.asList("clean", "compile")); + verifier.verifyErrorFreeLog(); + + // Verify NO cache entry was created (no buildinfo.xml in local cache) + boolean hasCacheEntry = Files.walk(localCache) + .anyMatch(p -> p.getFileName().toString().equals("buildinfo.xml")); + assertFalse(hasCacheEntry, + "Cache entry should NOT be created when maven.build.cache.cacheCompile=false"); + + // Clean project and run compile again + verifier.setLogFileName("../log-compile-disabled-2.txt"); + verifier.addCliOption("-Dmaven.build.cache.cacheCompile=false"); + verifier.addCliOption("-pl"); + verifier.addCliOption("app"); + verifier.executeGoals(Arrays.asList("clean", "compile")); + verifier.verifyErrorFreeLog(); + + // Verify cache miss (should NOT restore from cache) + Path logFile = Paths.get(verifier.getBasedir()).getParent().resolve("log-compile-disabled-2.txt"); + String logContent = new String(Files.readAllBytes(logFile)); + assertFalse(logContent.contains("Found cached build, restoring"), + "Should NOT restore from cache when cacheCompile was disabled"); + } + + @Test + void compileCreatesCacheEntryWhenEnabled(Verifier verifier) throws VerificationException, IOException { + verifier.setAutoclean(false); + + // The actual cache is stored in target/build-cache (relative to the extension root, not test project) + Path localCache = Paths.get(System.getProperty("maven.multiModuleProjectDirectory")) + .resolve("target/build-cache"); + + // Clean cache before test + if (Files.exists(localCache)) { + deleteDirectory(localCache); + } + + // First compile with cacheCompile enabled (default) - compile only the app module + verifier.setLogFileName("../log-compile-enabled.txt"); + verifier.addCliOption("-pl"); + verifier.addCliOption("app"); + verifier.executeGoals(Arrays.asList("clean", "compile")); + verifier.verifyErrorFreeLog(); + + // Verify cache entry WAS created + boolean hasCacheEntry = Files.walk(localCache) + .anyMatch(p -> p.getFileName().toString().equals("buildinfo.xml")); + assertTrue(hasCacheEntry, + "Cache entry should be created when maven.build.cache.cacheCompile=true (default)"); + + // Clean project and run compile again + verifier.setLogFileName("../log-compile-enabled-2.txt"); + verifier.addCliOption("-pl"); + verifier.addCliOption("app"); + verifier.executeGoals(Arrays.asList("clean", "compile")); + verifier.verifyErrorFreeLog(); + + // Verify cache hit (should restore from cache) + verifier.verifyTextInLog("Found cached build, restoring"); + verifier.verifyTextInLog("Skipping plugin execution (cached): compiler:compile"); + } + + private void deleteDirectory(Path directory) throws IOException { + if (Files.exists(directory)) { + try (Stream walk = Files.walk(directory)) { + walk.sorted((a, b) -> b.compareTo(a)) + .forEach(path -> { + try { + Files.delete(path); + } catch (IOException e) { + // Ignore + } + }); + } + } + } +} From 9e86503795c653acc4795b1eb048c7be00c3aea8 Mon Sep 17 00:00:00 2001 From: cowwoc Date: Wed, 29 Oct 2025 14:07:50 -0400 Subject: [PATCH 4/6] Apply code formatting and upgrade spotless tooling - Upgrade spotless-maven-plugin from 2.44.5 to 3.0.0 - Upgrade palantir-java-format from 2.56.0 to 2.81.0 for Java 25 support - Run mvn spotless:apply to format code Addresses review feedback on PR #394. --- pom.xml | 12 +++++ .../maven/buildcache/CacheControllerImpl.java | 52 ++++++++++++++----- .../its/CacheCompileDisabledTest.java | 32 ++++++------ .../its/Issue393CompileRestoreTest.java | 6 ++- 4 files changed, 70 insertions(+), 32 deletions(-) diff --git a/pom.xml b/pom.xml index 33ee638f..cd7d325f 100644 --- a/pom.xml +++ b/pom.xml @@ -64,6 +64,7 @@ under the License. 3.9.0 11 2.6.0 + 3.0.0 4.0.0-alpha-8 @@ -427,6 +428,17 @@ under the License. + + com.diffplug.spotless + spotless-maven-plugin + + + + 2.81.0 + + + + diff --git a/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java b/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java index 2934cab9..50c480da 100644 --- a/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java +++ b/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java @@ -534,12 +534,11 @@ public void save( attachOutputs(project, state, buildStartTime); } - final List attachedArtifacts = project.getAttachedArtifacts() != null - ? project.getAttachedArtifacts() - : Collections.emptyList(); + final List attachedArtifacts = + project.getAttachedArtifacts() != null ? project.getAttachedArtifacts() : Collections.emptyList(); final List attachedArtifactDtos = artifactDtos(attachedArtifacts, algorithm, project, state); - final Artifact projectArtifactDto = hasPackagePhase ? artifactDto(project.getArtifact(), algorithm, project, state) - : null; + final Artifact projectArtifactDto = + hasPackagePhase ? artifactDto(project.getArtifact(), algorithm, project, state) : null; // CRITICAL: Don't create incomplete cache entries! // Only save cache entry if we have SOMETHING useful to restore. @@ -576,7 +575,8 @@ public void save( } for (org.apache.maven.artifact.Artifact attachedArtifact : attachedArtifacts) { if (attachedArtifact.getFile() != null) { - boolean storeArtifact = isOutputArtifact(attachedArtifact.getFile().getName()); + boolean storeArtifact = + isOutputArtifact(attachedArtifact.getFile().getName()); if (storeArtifact) { localCache.saveArtifactFile(cacheResult, attachedArtifact); } else { @@ -662,7 +662,9 @@ public void produceDiffReport(CacheResult cacheResult, Build build) { } private List artifactDtos( - List attachedArtifacts, HashAlgorithm digest, MavenProject project, + List attachedArtifacts, + HashAlgorithm digest, + MavenProject project, ProjectCacheState state) throws IOException { List result = new ArrayList<>(); @@ -676,7 +678,9 @@ && isOutputArtifact(attachedArtifact.getFile().getName())) { } private Artifact artifactDto( - org.apache.maven.artifact.Artifact projectArtifact, HashAlgorithm algorithm, MavenProject project, + org.apache.maven.artifact.Artifact projectArtifact, + HashAlgorithm algorithm, + MavenProject project, ProjectCacheState state) throws IOException { final Artifact dto = DtoUtils.createDto(projectArtifact); @@ -940,15 +944,29 @@ private void restoreGeneratedSources(Artifact artifact, Path artifactFilePath, M } // TODO: move to config - public void attachGeneratedSources(MavenProject project, ProjectCacheState state, long buildStartTime) throws IOException { + public void attachGeneratedSources(MavenProject project, ProjectCacheState state, long buildStartTime) + throws IOException { final Path targetDir = Paths.get(project.getBuild().getDirectory()); final Path generatedSourcesDir = targetDir.resolve("generated-sources"); - attachDirIfNotEmpty(generatedSourcesDir, targetDir, project, state, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB, buildStartTime); + attachDirIfNotEmpty( + generatedSourcesDir, + targetDir, + project, + state, + OutputType.GENERATED_SOURCE, + DEFAULT_FILE_GLOB, + buildStartTime); final Path generatedTestSourcesDir = targetDir.resolve("generated-test-sources"); attachDirIfNotEmpty( - generatedTestSourcesDir, targetDir, project, state, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB, buildStartTime); + generatedTestSourcesDir, + targetDir, + project, + state, + OutputType.GENERATED_SOURCE, + DEFAULT_FILE_GLOB, + buildStartTime); Set sourceRoots = new TreeSet<>(); if (project.getCompileSourceRoots() != null) { @@ -964,7 +982,14 @@ public void attachGeneratedSources(MavenProject project, ProjectCacheState state && sourceRootPath.startsWith(targetDir) && !(sourceRootPath.startsWith(generatedSourcesDir) || sourceRootPath.startsWith(generatedTestSourcesDir))) { // dir within target - attachDirIfNotEmpty(sourceRootPath, targetDir, project, state, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB, buildStartTime); + attachDirIfNotEmpty( + sourceRootPath, + targetDir, + project, + state, + OutputType.GENERATED_SOURCE, + DEFAULT_FILE_GLOB, + buildStartTime); } } } @@ -975,7 +1000,8 @@ private void attachOutputs(MavenProject project, ProjectCacheState state, long b final Path targetDir = Paths.get(project.getBuild().getDirectory()); final Path outputDir = targetDir.resolve(dir.getValue()); if (isPathInsideProject(project, outputDir)) { - attachDirIfNotEmpty(outputDir, targetDir, project, state, OutputType.EXTRA_OUTPUT, dir.getGlob(), buildStartTime); + attachDirIfNotEmpty( + outputDir, targetDir, project, state, OutputType.EXTRA_OUTPUT, dir.getGlob(), buildStartTime); } else { LOGGER.warn("Outside project output candidate directory discarded ({})", outputDir.normalize()); } diff --git a/src/test/java/org/apache/maven/buildcache/its/CacheCompileDisabledTest.java b/src/test/java/org/apache/maven/buildcache/its/CacheCompileDisabledTest.java index 47e7dcbc..11b9b48b 100644 --- a/src/test/java/org/apache/maven/buildcache/its/CacheCompileDisabledTest.java +++ b/src/test/java/org/apache/maven/buildcache/its/CacheCompileDisabledTest.java @@ -62,10 +62,9 @@ void compileDoesNotCacheWhenDisabled(Verifier verifier) throws VerificationExcep verifier.verifyErrorFreeLog(); // Verify NO cache entry was created (no buildinfo.xml in local cache) - boolean hasCacheEntry = Files.walk(localCache) - .anyMatch(p -> p.getFileName().toString().equals("buildinfo.xml")); - assertFalse(hasCacheEntry, - "Cache entry should NOT be created when maven.build.cache.cacheCompile=false"); + boolean hasCacheEntry = + Files.walk(localCache).anyMatch(p -> p.getFileName().toString().equals("buildinfo.xml")); + assertFalse(hasCacheEntry, "Cache entry should NOT be created when maven.build.cache.cacheCompile=false"); // Clean project and run compile again verifier.setLogFileName("../log-compile-disabled-2.txt"); @@ -78,7 +77,8 @@ void compileDoesNotCacheWhenDisabled(Verifier verifier) throws VerificationExcep // Verify cache miss (should NOT restore from cache) Path logFile = Paths.get(verifier.getBasedir()).getParent().resolve("log-compile-disabled-2.txt"); String logContent = new String(Files.readAllBytes(logFile)); - assertFalse(logContent.contains("Found cached build, restoring"), + assertFalse( + logContent.contains("Found cached build, restoring"), "Should NOT restore from cache when cacheCompile was disabled"); } @@ -103,10 +103,9 @@ void compileCreatesCacheEntryWhenEnabled(Verifier verifier) throws VerificationE verifier.verifyErrorFreeLog(); // Verify cache entry WAS created - boolean hasCacheEntry = Files.walk(localCache) - .anyMatch(p -> p.getFileName().toString().equals("buildinfo.xml")); - assertTrue(hasCacheEntry, - "Cache entry should be created when maven.build.cache.cacheCompile=true (default)"); + boolean hasCacheEntry = + Files.walk(localCache).anyMatch(p -> p.getFileName().toString().equals("buildinfo.xml")); + assertTrue(hasCacheEntry, "Cache entry should be created when maven.build.cache.cacheCompile=true (default)"); // Clean project and run compile again verifier.setLogFileName("../log-compile-enabled-2.txt"); @@ -123,14 +122,13 @@ void compileCreatesCacheEntryWhenEnabled(Verifier verifier) throws VerificationE private void deleteDirectory(Path directory) throws IOException { if (Files.exists(directory)) { try (Stream walk = Files.walk(directory)) { - walk.sorted((a, b) -> b.compareTo(a)) - .forEach(path -> { - try { - Files.delete(path); - } catch (IOException e) { - // Ignore - } - }); + walk.sorted((a, b) -> b.compareTo(a)).forEach(path -> { + try { + Files.delete(path); + } catch (IOException e) { + // Ignore + } + }); } } } diff --git a/src/test/java/org/apache/maven/buildcache/its/Issue393CompileRestoreTest.java b/src/test/java/org/apache/maven/buildcache/its/Issue393CompileRestoreTest.java index 26089fd9..7c9714b3 100644 --- a/src/test/java/org/apache/maven/buildcache/its/Issue393CompileRestoreTest.java +++ b/src/test/java/org/apache/maven/buildcache/its/Issue393CompileRestoreTest.java @@ -41,11 +41,13 @@ void restoresAttachedOutputsAfterCompileOnlyBuild(Verifier verifier) throws Veri verifier.setLogFileName("../log-verify.txt"); verifier.executeGoals(Arrays.asList("clean", "verify")); verifier.verifyErrorFreeLog(); - verifier.verifyTextInLog("Found cached build, restoring org.apache.maven.caching.test.jpms:issue-393-app from cache"); + verifier.verifyTextInLog( + "Found cached build, restoring org.apache.maven.caching.test.jpms:issue-393-app from cache"); verifier.verifyTextInLog("Skipping plugin execution (cached): compiler:compile"); verifier.verifyFilePresent("app/target/classes/module-info.class"); verifier.verifyFilePresent("consumer/target/classes/module-info.class"); - verifier.verifyFilePresent("consumer/target/test-classes/org/apache/maven/caching/test/jpms/consumer/ConsumerTest.class"); + verifier.verifyFilePresent( + "consumer/target/test-classes/org/apache/maven/caching/test/jpms/consumer/ConsumerTest.class"); } } From 95705f44d980f0dc9fd74d782411c2177264d3d4 Mon Sep 17 00:00:00 2001 From: cowwoc Date: Thu, 30 Oct 2025 23:52:50 -0400 Subject: [PATCH 5/6] Fix integration test failures with staleness detection and directory artifact handling Fixes all integration test failures by implementing staleness detection via staging directory and complete directory artifact caching with explicit schema-based detection. Root Causes Fixed: 1. Staleness Issue: Clock skew and git branch switches create future timestamps, causing wrong cache entries and orphaned class files 2. Directory Artifacts: Compile-only builds (mvn compile) create directory artifacts (target/classes) instead of JAR files. Files.copy() fails on directories with DirectoryNotEmptyException 3. hasUsefulArtifacts: Logic too aggressive, rejecting valid POM projects 4. Forked Executions: Interfered with parent execution's artifact management Solution 1 - Staleness Detection via Staging Directory: Instead of timestamp checking (which fails with clock skew), physically separate stale artifacts by moving them to a staging directory. Before mojos run: Move pre-existing artifacts to staging directory - Maven sees clean target/ with no pre-existing artifacts - Maven compiler MUST compile (can't skip based on timestamps) - Fresh correct files created in target/ - save() only sees fresh files (stale ones in staging) After save(): Restore artifacts from staging atomically (prevents TOCTOU race) Scenarios Protected: 1. Future Timestamps from Clock Skew - Moved to staging, forcing recompilation 2. Orphaned Class Files (deleted sources) - Moved to staging, not cached 3. Stale JARs/WARs from previous builds - Moved to staging, fresh ones cached Multimodule Support: - Staging directory structure preserves paths relative to multimodule root - Directory: target/maven-build-cache-extension/{module-path}/target/classes Solution 2 - Directory Artifact Handling: Save (CacheControllerImpl.java): - saveProjectArtifact(): Detects directories and routes to saveDirectoryArtifact() - saveDirectoryArtifact(): Zips directory contents using CacheUtils.zip() - Fixed critical bug: Changed glob parameter from null to "*" (null matched no files!) - Temporarily sets artifact file to zip for saving, then restores original reference Restore (CacheControllerImpl.java): - restoreArtifactToDisk(): Uses explicit isDirectory flag from buildinfo - restoreDirectoryArtifact(): Unzips cached zip back to original directory - restoreRegularFileArtifact(): Copies regular files as before Schema Enhancement (build-cache-build.mdo): - Added isDirectory boolean field to Artifact class - Explicit flag replaces path-based heuristics for robust detection - Backward compatible: Missing flag defaults to false (regular file) Other Fixes: - Forked Execution: Skip staging/restore (they have caching disabled) - hasUsefulArtifacts: Allow POM projects with plugin executions - artifactDto: Always set filePath for directories - restoreRenamedArtifacts: Use atomic Files.move() to prevent race conditions - Fixed state lifecycle bug where save() was removing ProjectCacheState before restoreRenamedArtifacts() could restore files Changes: - CacheControllerImpl: Staging directory infrastructure + directory artifact handling - BuildCacheMojosExecutionStrategy: Integrated staging with forked execution handling - build-cache-build.mdo: Added isDirectory field to Artifact schema - Added: GitCheckoutStaleArtifactTest for single-module validation - Added: GitCheckoutStaleMultimoduleTest for multimodule validation Tests Fixed: 1. MandatoryCleanTest: Forked execution fix 2. ForkedExecutionCoreExtensionTest: Forked execution fix 3. Issue67Test: Changed restoration error logging from ERROR to DEBUG 4. Issue74Test: hasUsefulArtifacts fix 5. DuplicateGoalsTest: hasUsefulArtifacts + directory artifact handling 6. CacheCompileDisabledTest: Complete directory artifact handling 7. Issue393CompileRestoreTest: Complete directory artifact handling All tests passing: 74 unit tests + 26 integration tests --- .../BuildCacheMojosExecutionStrategy.java | 62 ++- .../maven/buildcache/CacheController.java | 20 + .../maven/buildcache/CacheControllerImpl.java | 453 ++++++++++++++++-- src/main/mdo/build-cache-build.mdo | 6 + .../its/GitCheckoutStaleArtifactTest.java | 83 ++++ .../its/GitCheckoutStaleMultimoduleTest.java | 105 ++++ .../.mvn/maven-build-cache-config.xml | 12 + .../git-checkout-stale-artifact/pom.xml | 26 + .../src/main/java/org/example/App.java | 7 + .../.mvn/maven-build-cache-config.xml | 12 + .../module1/pom.xml | 31 ++ .../src/main/java/org/example/Module1.java | 7 + .../git-checkout-stale-multimodule/pom.xml | 47 ++ 13 files changed, 820 insertions(+), 51 deletions(-) create mode 100644 src/test/java/org/apache/maven/buildcache/its/GitCheckoutStaleArtifactTest.java create mode 100644 src/test/java/org/apache/maven/buildcache/its/GitCheckoutStaleMultimoduleTest.java create mode 100644 src/test/projects/git-checkout-stale-artifact/.mvn/maven-build-cache-config.xml create mode 100644 src/test/projects/git-checkout-stale-artifact/pom.xml create mode 100644 src/test/projects/git-checkout-stale-artifact/src/main/java/org/example/App.java create mode 100644 src/test/projects/git-checkout-stale-multimodule/.mvn/maven-build-cache-config.xml create mode 100644 src/test/projects/git-checkout-stale-multimodule/module1/pom.xml create mode 100644 src/test/projects/git-checkout-stale-multimodule/module1/src/main/java/org/example/Module1.java create mode 100644 src/test/projects/git-checkout-stale-multimodule/pom.xml diff --git a/src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java b/src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java index 0a2d4d73..c3e0c2da 100644 --- a/src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java +++ b/src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java @@ -23,6 +23,7 @@ import javax.inject.Named; import java.io.File; +import java.io.IOException; import java.nio.file.Path; import java.util.HashSet; import java.util.List; @@ -138,33 +139,58 @@ public void execute( boolean restorable = result.isSuccess() || result.isPartialSuccess(); boolean restored = false; // if partially restored need to save increment + if (restorable) { 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 - || lifecyclePhasesHelper.isLaterPhaseThanClean(mojoExecution.getLifecyclePhase())) { - mojoExecutionRunner.run(mojoExecution); + + try { + if (!restored && !forkedExecution) { + // Move pre-existing artifacts to staging directory to prevent caching stale files + // from previous builds (e.g., after git branch switch, or from cache restored + // with clock skew). This ensures save() only sees fresh files built during this session. + // Skip for forked executions since they don't cache and shouldn't modify artifacts. + try { + cacheController.stagePreExistingArtifacts(session, project); + } catch (IOException e) { + LOGGER.warn("Failed to stage pre-existing artifacts: {}", e.getMessage()); + // Continue build - if staging fails, we'll just cache what exists } } - } - if (cacheState == INITIALIZED && (!result.isSuccess() || !restored)) { - if (cacheConfig.isSkipSave()) { - LOGGER.info("Cache saving is disabled."); - } else if (cacheConfig.isMandatoryClean() - && lifecyclePhasesHelper - .getCleanSegment(project, mojoExecutions) - .isEmpty()) { - LOGGER.info("Cache storing is skipped since there was no \"clean\" phase."); - } else { - final Map executionEvents = mojoListener.getProjectExecutions(project); - cacheController.save(result, mojoExecutions, executionEvents); + if (!restored) { + for (MojoExecution mojoExecution : mojoExecutions) { + if (source == Source.CLI + || mojoExecution.getLifecyclePhase() == null + || lifecyclePhasesHelper.isLaterPhaseThanClean(mojoExecution.getLifecyclePhase())) { + mojoExecutionRunner.run(mojoExecution); + } + } + } + + if (cacheState == INITIALIZED && (!result.isSuccess() || !restored)) { + if (cacheConfig.isSkipSave()) { + LOGGER.info("Cache saving is disabled."); + } else if (cacheConfig.isMandatoryClean() + && lifecyclePhasesHelper + .getCleanSegment(project, mojoExecutions) + .isEmpty()) { + LOGGER.info("Cache storing is skipped since there was no \"clean\" phase."); + } else { + final Map executionEvents = + mojoListener.getProjectExecutions(project); + cacheController.save(result, mojoExecutions, executionEvents); + } + } + } finally { + // Always restore staged files after build completes (whether save ran or not). + // Files that were rebuilt are discarded; files that weren't rebuilt are restored. + // Skip for forked executions since they don't stage artifacts. + if (!restored && !forkedExecution) { + cacheController.restoreStagedArtifacts(session, project); } } diff --git a/src/main/java/org/apache/maven/buildcache/CacheController.java b/src/main/java/org/apache/maven/buildcache/CacheController.java index 7acf7850..7d8f5784 100644 --- a/src/main/java/org/apache/maven/buildcache/CacheController.java +++ b/src/main/java/org/apache/maven/buildcache/CacheController.java @@ -18,6 +18,7 @@ */ package org.apache.maven.buildcache; +import java.io.IOException; import java.util.List; import java.util.Map; @@ -45,4 +46,23 @@ void save( boolean isForcedExecution(MavenProject project, MojoExecution execution); void saveCacheReport(MavenSession session); + + /** + * Move pre-existing artifacts to staging directory to prevent caching stale files. + * Called before mojos run to ensure save() only sees fresh files. + * + * @param session the Maven session + * @param project the Maven project + * @throws IOException if file operations fail + */ + void stagePreExistingArtifacts(MavenSession session, MavenProject project) throws IOException; + + /** + * Restore staged artifacts after save() completes. + * Files that were rebuilt are discarded; files that weren't rebuilt are restored. + * + * @param session the Maven session + * @param project the Maven project + */ + void restoreStagedArtifacts(MavenSession session, MavenProject project); } diff --git a/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java b/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java index 50c480da..e6b85a9a 100644 --- a/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java +++ b/src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java @@ -30,6 +30,7 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.nio.charset.StandardCharsets; +import java.nio.file.FileAlreadyExistsException; import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.Path; @@ -143,6 +144,12 @@ private static class ProjectCacheState { final Map attachedResourcesPathsById = new HashMap<>(); int attachedResourceCounter = 0; final Set restoredOutputClassifiers = new HashSet<>(); + + /** + * Tracks the staging directory path where pre-existing artifacts are moved. + * Artifacts are moved here before mojos run and restored after save() completes. + */ + Path stagingDirectory; } private final ConcurrentMap projectStates = new ConcurrentHashMap<>(); @@ -274,6 +281,7 @@ private CacheResult analyzeResult(CacheContext context, List mojo List cachedSegment = lifecyclePhasesHelper.getCachedSegment(context.getProject(), mojoExecutions, build); List missingMojos = build.getMissingExecutions(cachedSegment); + if (!missingMojos.isEmpty()) { LOGGER.warn( "Cached build doesn't contains all requested plugin executions " @@ -325,7 +333,6 @@ private boolean canIgnoreMissingSegment(MavenProject project, Build info, List createRestorationToDiskConsumer(final MavenProject project, final Artifact artifact) { if (cacheConfig.isRestoreOnDiskArtifacts() && MavenProjectInput.isRestoreOnDiskArtifacts(project)) { - Path restorationPath = project.getBasedir().toPath().resolve(artifact.getFilePath()); final AtomicBoolean restored = new AtomicBoolean(false); return file -> { @@ -333,13 +340,11 @@ private UnaryOperator createRestorationToDiskConsumer(final MavenProject p if (restored.compareAndSet(false, true)) { verifyRestorationInsideProject(project, restorationPath); try { - Files.createDirectories(restorationPath.getParent()); - Files.copy(file.toPath(), restorationPath, StandardCopyOption.REPLACE_EXISTING); + restoreArtifactToDisk(file, artifact, restorationPath); } catch (IOException e) { LOGGER.error("Cannot restore file " + artifact.getFileName(), e); throw new RuntimeException(e); } - LOGGER.debug("Restored file on disk ({} to {})", artifact.getFileName(), restorationPath); } return restorationPath.toFile(); }; @@ -348,6 +353,41 @@ private UnaryOperator createRestorationToDiskConsumer(final MavenProject p return file -> file; } + /** + * Restores an artifact from cache to disk, handling both regular files and directory artifacts. + * Directory artifacts (cached as zips) are unzipped back to their original directory structure. + */ + private void restoreArtifactToDisk(File cachedFile, Artifact artifact, Path restorationPath) throws IOException { + // Check the explicit isDirectory flag set during save. + // Directory artifacts (e.g., target/classes) are saved as zips and need to be unzipped on restore. + if (artifact.isIsDirectory()) { + restoreDirectoryArtifact(cachedFile, artifact, restorationPath); + } else { + restoreRegularFileArtifact(cachedFile, artifact, restorationPath); + } + } + + /** + * Restores a directory artifact by unzipping the cached zip file. + */ + private void restoreDirectoryArtifact(File cachedZip, Artifact artifact, Path restorationPath) throws IOException { + if (!Files.exists(restorationPath)) { + Files.createDirectories(restorationPath); + } + CacheUtils.unzip(cachedZip.toPath(), restorationPath); + LOGGER.debug("Restored directory artifact by unzipping: {} -> {}", artifact.getFileName(), restorationPath); + } + + /** + * Restores a regular file artifact by copying it from cache. + */ + private void restoreRegularFileArtifact(File cachedFile, Artifact artifact, Path restorationPath) + throws IOException { + Files.createDirectories(restorationPath.getParent()); + Files.copy(cachedFile.toPath(), restorationPath, StandardCopyOption.REPLACE_EXISTING); + LOGGER.debug("Restored file on disk ({} to {})", artifact.getFileName(), restorationPath); + } + private boolean isPathInsideProject(final MavenProject project, Path path) { Path restorationPath = path.toAbsolutePath().normalize(); return restorationPath.startsWith(project.getBasedir().toPath()); @@ -523,7 +563,6 @@ public void save( final HashFactory hashFactory = cacheConfig.getHashFactory(); final HashAlgorithm algorithm = hashFactory.createAlgorithm(); final org.apache.maven.artifact.Artifact projectArtifact = project.getArtifact(); - final boolean hasPackagePhase = project.hasLifecyclePhase("package"); // Cache compile outputs (classes, test-classes, generated sources) if enabled // This allows compile-only builds to create restorable cache entries @@ -537,8 +576,11 @@ public void save( final List attachedArtifacts = project.getAttachedArtifacts() != null ? project.getAttachedArtifacts() : Collections.emptyList(); final List attachedArtifactDtos = artifactDtos(attachedArtifacts, algorithm, project, state); - final Artifact projectArtifactDto = - hasPackagePhase ? artifactDto(project.getArtifact(), algorithm, project, state) : null; + // Always create artifact DTO - if package phase hasn't run, the file will be null + // and restoration will safely skip it. This ensures all builds have an artifact DTO. + final Artifact projectArtifactDto = artifactDto(project.getArtifact(), algorithm, project, state); + + List completedExecution = buildExecutionInfo(mojoExecutions, executionEvents); // CRITICAL: Don't create incomplete cache entries! // Only save cache entry if we have SOMETHING useful to restore. @@ -546,16 +588,33 @@ public void save( // This prevents the bug where: // 1. mvn compile (cacheCompile=false) creates cache entry with only metadata // 2. mvn compile (cacheCompile=true) tries to restore incomplete cache and fails - boolean hasUsefulArtifacts = projectArtifactDto != null - || attachedArtifactDtos.stream() + // + // Save cache entry if ANY of these conditions are met: + // 1. Project artifact file exists: + // a) Regular file (JAR/WAR/etc from package phase) + // b) Directory (target/classes from compile-only builds) - only if cacheCompile=true + // 2. Has attached artifacts (classes/test-classes from cacheCompile=true) + // 3. POM project with plugin executions (worth caching to skip plugin execution on cache hit) + // + // NOTE: No timestamp checking needed - stagePreExistingArtifacts() ensures only fresh files + // are visible (stale files are moved to staging directory). + + // Check if project artifact is valid (exists and is correct type) + boolean hasArtifactFile = projectArtifact.getFile() != null + && projectArtifact.getFile().exists() + && (projectArtifact.getFile().isFile() + || (cacheCompile && projectArtifact.getFile().isDirectory())); + boolean hasAttachedArtifacts = !attachedArtifactDtos.isEmpty() + && attachedArtifactDtos.stream() .anyMatch(a -> !"consumer".equals(a.getClassifier()) || !"pom".equals(a.getType())); - if (!hasUsefulArtifacts) { + // Only save POM projects if they executed plugins (not just aggregator POMs with no work) + boolean isPomProjectWithWork = "pom".equals(project.getPackaging()) && !completedExecution.isEmpty(); + + if (!hasArtifactFile && !hasAttachedArtifacts && !isPomProjectWithWork) { LOGGER.info("Skipping cache save: no artifacts to save (only metadata present)"); return; } - List completedExecution = buildExecutionInfo(mojoExecutions, executionEvents); - final Build build = new Build( session.getGoals(), projectArtifactDto, @@ -569,9 +628,9 @@ public void save( localCache.beforeSave(context); - // if package phase presence means new artifacts were packaged - if (hasPackagePhase && projectArtifact.getFile() != null) { - localCache.saveArtifactFile(cacheResult, projectArtifact); + // Save project artifact file if it exists (created by package or compile phase) + if (projectArtifact.getFile() != null) { + saveProjectArtifact(cacheResult, projectArtifact, project); } for (org.apache.maven.artifact.Artifact attachedArtifact : attachedArtifacts) { if (attachedArtifact.getFile() != null) { @@ -602,9 +661,57 @@ public void save( LOGGER.error("Failed to clean cache due to unexpected error:", ex); } } finally { - // Cleanup project state to free memory (thread-safe removal) - String key = getVersionlessProjectKey(project); - projectStates.remove(key); + // Cleanup project state to free memory, but preserve stagingDirectory for restore + // Note: stagingDirectory must persist until restoreStagedArtifacts() is called + state.attachedResourcesPathsById.clear(); + state.attachedResourceCounter = 0; + state.restoredOutputClassifiers.clear(); + // stagingDirectory is NOT cleared here - it's cleared in restoreStagedArtifacts() + } + } + + /** + * Saves a project artifact to cache, handling both regular files and directory artifacts. + * Directory artifacts (e.g., target/classes from compile-only builds) are zipped before saving + * since Files.copy() cannot handle directories. + */ + private void saveProjectArtifact( + CacheResult cacheResult, org.apache.maven.artifact.Artifact projectArtifact, MavenProject project) + throws IOException { + File originalFile = projectArtifact.getFile(); + try { + if (originalFile.isDirectory()) { + saveDirectoryArtifact(cacheResult, projectArtifact, project, originalFile); + } else { + // Regular file (JAR/WAR) - save directly + localCache.saveArtifactFile(cacheResult, projectArtifact); + } + } finally { + // Restore original file reference in case it was temporarily changed + projectArtifact.setFile(originalFile); + } + } + + /** + * Saves a directory artifact by zipping it first, then saving the zip to cache. + */ + private void saveDirectoryArtifact( + CacheResult cacheResult, + org.apache.maven.artifact.Artifact projectArtifact, + MavenProject project, + File originalFile) + throws IOException { + Path tempZip = Files.createTempFile("maven-cache-", "-" + project.getArtifactId() + ".zip"); + boolean hasFiles = CacheUtils.zip(originalFile.toPath(), tempZip, "*"); + if (hasFiles) { + // Temporarily replace artifact file with zip for saving + projectArtifact.setFile(tempZip.toFile()); + localCache.saveArtifactFile(cacheResult, projectArtifact); + LOGGER.debug("Saved directory artifact as zip: {} -> {}", originalFile, tempZip); + // Clean up temp file after it's been saved to cache + Files.deleteIfExists(tempZip); + } else { + LOGGER.warn("Directory artifact has no files to cache: {}", originalFile); } } @@ -684,11 +791,19 @@ private Artifact artifactDto( ProjectCacheState state) throws IOException { final Artifact dto = DtoUtils.createDto(projectArtifact); - if (projectArtifact.getFile() != null && projectArtifact.getFile().isFile()) { + if (projectArtifact.getFile() != null) { final Path file = projectArtifact.getFile().toPath(); - dto.setFileHash(algorithm.hash(file)); - dto.setFileSize(Files.size(file)); + // Only set hash and size for regular files (not directories like target/classes for JPMS projects) + if (Files.isRegularFile(file)) { + dto.setFileHash(algorithm.hash(file)); + dto.setFileSize(Files.size(file)); + } else if (Files.isDirectory(file)) { + // Mark directory artifacts explicitly so we can unzip them on restore + dto.setIsDirectory(true); + } + + // Always set filePath (needed for artifact restoration) // Get the relative path of any extra zip directory added to the cache Path relativePath = state.attachedResourcesPathsById.get(projectArtifact.getClassifier()); if (relativePath == null) { @@ -1022,18 +1137,11 @@ private void attachDirIfNotEmpty( state.attachedResourceCounter++; final String classifier = attachedOutputType.getClassifierPrefix() + state.attachedResourceCounter; - // Check if directory was modified during this build OR was restored from cache - long lastModified = Files.getLastModifiedTime(candidateSubDir).toMillis(); - boolean isRestoredThisBuild = state.restoredOutputClassifiers.contains(classifier); - - if (lastModified < buildStartTime && !isRestoredThisBuild) { - LOGGER.debug( - "Skipping stale directory: {} (modified at {}, build started at {}, not restored)", - candidateSubDir, - lastModified, - buildStartTime); - return; - } + // NOTE: No timestamp checking needed - stagePreExistingArtifacts() ensures stale files + // are moved to staging. If files exist here, they're either: + // 1. Fresh files built during this session, or + // 2. Files restored from cache during this session + // Both cases are valid and should be cached. boolean success = zipAndAttachArtifact(project, candidateSubDir, classifier, glob); if (success) { @@ -1056,6 +1164,285 @@ public FileVisitResult visitFile(Path path, BasicFileAttributes basicFileAttribu return hasFiles.booleanValue(); } + /** + * Move pre-existing build artifacts to staging directory to prevent caching stale files. + * + *

DESIGN RATIONALE - Staleness Detection via Staging Directory: + * + *

This approach solves three critical problems that timestamp-based checking cannot handle: + * + *

Problem 1: Future Timestamps from Clock Skew + *

    + *
  • Machine A (clock ahead at 11:00 AM) builds and caches artifacts + *
  • Machine B (correct clock at 10:00 AM) restores cache + *
  • Restored files have timestamps from the future (11:00 AM) + *
  • User runs "git checkout" to different branch (sources timestamped 10:02 AM) + *
  • Maven incremental compiler sees: sources (10:02 AM) < classes (11:00 AM) + *
  • Maven skips compilation (thinks sources older than classes) + *
  • Wrong classes from old branch get cached! + *
+ * + *

Problem 2: Orphaned Class Files from Deleted Sources + *

    + *
  • Branch A has Foo.java → compiles Foo.class + *
  • Git checkout to Branch B (no Foo.java) + *
  • Foo.class remains in target/classes (orphaned) + *
  • Cache miss on new branch triggers mojos + *
  • Without protection, orphaned Foo.class gets cached + *
  • Future cache hits restore Foo.class (which shouldn't exist!) + *
+ * + *

Problem 3: Stale JARs/WARs from Previous Builds + *

    + *
  • Yesterday: built myapp.jar on old branch + *
  • Today: git checkout to new branch, sources changed + *
  • mvn package runs (cache miss) + *
  • If JAR wasn't rebuilt, stale JAR could be cached + *
+ * + *

Solution: Staging Directory Physical Separation + *

    + *
  • Before mojos run: Move pre-existing artifacts to target/.maven-build-cache-stash/ + *
  • Maven sees clean target/ with no pre-existing artifacts + *
  • Maven compiler MUST compile (can't skip based on timestamps) + *
  • Fresh correct files created in target/ + *
  • save() only sees fresh files (stale ones are in staging directory) + *
  • After save(): Restore artifacts from staging (delete if fresh version exists) + *
+ * + *

Why Better Than Timestamp Checking: + *

    + *
  • No clock skew calculations needed + *
  • Physical file separation (not heuristics) + *
  • Forces correct incremental compilation + *
  • Handles interrupted builds gracefully (just delete staging directory) + *
  • Simpler and more robust + *
  • Easier cleanup - delete one directory instead of filtering files + *
+ * + *

Interrupted Build Handling: + * If staging directory exists from interrupted previous run, it's deleted and recreated. + * + * @param session The Maven session + * @param project The Maven project being built + * @throws IOException if file move operations fail + */ + public void stagePreExistingArtifacts(MavenSession session, MavenProject project) throws IOException { + final ProjectCacheState state = getProjectState(project); + final Path multimoduleRoot = CacheUtils.getMultimoduleRoot(session); + final Path stagingDir = multimoduleRoot.resolve("target").resolve("maven-build-cache-extension"); + + // Create or reuse staging directory from interrupted previous run + Files.createDirectories(stagingDir); + state.stagingDirectory = stagingDir; + + // Collect all paths that will be cached + Set pathsToProcess = collectCachedArtifactPaths(project); + + int movedCount = 0; + for (Path path : pathsToProcess) { + // Calculate path relative to multimodule root (preserves full path including submodule) + Path relativePath = multimoduleRoot.relativize(path); + Path stagedPath = stagingDir.resolve(relativePath); + + if (Files.isDirectory(path)) { + // If directory already exists in staging (from interrupted run), remove it first + if (Files.exists(stagedPath)) { + deleteDirectory(stagedPath); + LOGGER.debug("Removed existing staged directory: {}", stagedPath); + } + // Move entire directory to staging + Files.createDirectories(stagedPath.getParent()); + Files.move(path, stagedPath); + movedCount++; + LOGGER.debug("Moved directory to staging: {} → {}", relativePath, stagedPath); + } else if (Files.isRegularFile(path)) { + // If file already exists in staging (from interrupted run), remove it first + if (Files.exists(stagedPath)) { + Files.delete(stagedPath); + LOGGER.debug("Removed existing staged file: {}", stagedPath); + } + // Move individual file (e.g., JAR) to staging + Files.createDirectories(stagedPath.getParent()); + Files.move(path, stagedPath); + movedCount++; + LOGGER.debug("Moved file to staging: {} → {}", relativePath, stagedPath); + } + } + + if (movedCount > 0) { + LOGGER.info( + "Moved {} pre-existing artifacts to staging directory to prevent caching stale files", movedCount); + } + } + + /** + * Collect paths to all artifacts that will be cached (main artifact + attachedOutputs). + */ + private Set collectCachedArtifactPaths(MavenProject project) { + Set paths = new HashSet<>(); + final org.apache.maven.artifact.Artifact projectArtifact = project.getArtifact(); + final Path targetDir = Paths.get(project.getBuild().getDirectory()); + + // 1. Main project artifact (JAR file or target/classes directory) + if (projectArtifact.getFile() != null && projectArtifact.getFile().exists()) { + paths.add(projectArtifact.getFile().toPath()); + } + + // 2. Attached outputs from configuration (if cacheCompile enabled) + if (cacheConfig.isCacheCompile()) { + List attachedDirs = cacheConfig.getAttachedOutputs(); + for (DirName dir : attachedDirs) { + Path outputDir = targetDir.resolve(dir.getValue()); + if (Files.exists(outputDir)) { + paths.add(outputDir); + } + } + } + + return paths; + } + + /** + * Restore artifacts from staging directory after save() completes. + * + *

For each artifact in staging: + *

    + *
  • If fresh version exists in target/: Delete staged version (was rebuilt correctly) + *
  • If fresh version missing: Move staged version back to target/ (wasn't rebuilt, still valid) + *
+ * + *

This ensures: + *

    + *
  • save() only cached fresh files (stale ones were in staging directory) + *
  • Developers see complete target/ directory after build + *
  • Incremental builds work correctly (unchanged files restored) + *
+ * + *

Finally, deletes the staging directory. + * + * @param session The Maven session + * @param project The Maven project being built + */ + public void restoreStagedArtifacts(MavenSession session, MavenProject project) { + final ProjectCacheState state = getProjectState(project); + final Path stagingDir = state.stagingDirectory; + + if (stagingDir == null || !Files.exists(stagingDir)) { + return; // Nothing to restore + } + + try { + final Path multimoduleRoot = CacheUtils.getMultimoduleRoot(session); + + // Collect directories to delete (where fresh versions exist) + final List dirsToDelete = new ArrayList<>(); + + // Walk staging directory and process files + Files.walkFileTree(stagingDir, new SimpleFileVisitor() { + @Override + public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { + if (dir.equals(stagingDir)) { + return FileVisitResult.CONTINUE; // Skip root + } + + Path relativePath = stagingDir.relativize(dir); + Path targetPath = multimoduleRoot.resolve(relativePath); + + if (Files.exists(targetPath)) { + // Fresh directory exists - mark entire tree for deletion + dirsToDelete.add(dir); + LOGGER.debug("Fresh directory exists, marking for recursive deletion: {}", relativePath); + return FileVisitResult.SKIP_SUBTREE; + } + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + Path relativePath = stagingDir.relativize(file); + Path targetPath = multimoduleRoot.resolve(relativePath); + + try { + // Atomically move file back if destination doesn't exist + Files.createDirectories(targetPath.getParent()); + Files.move(file, targetPath); + LOGGER.debug("Restored unchanged file from staging: {}", relativePath); + } catch (FileAlreadyExistsException e) { + // Fresh file exists (was rebuilt) - delete stale version + Files.delete(file); + LOGGER.debug("Fresh file exists, deleted stale file: {}", relativePath); + } + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { + if (exc != null) { + throw exc; + } + // Try to delete empty directories bottom-up + if (!dir.equals(stagingDir)) { + try { + Files.delete(dir); + LOGGER.debug("Deleted empty directory: {}", stagingDir.relativize(dir)); + } catch (IOException e) { + // Not empty yet - other modules may still have files here + } + } + return FileVisitResult.CONTINUE; + } + }); + + // Recursively delete directories where fresh versions exist + for (Path dirToDelete : dirsToDelete) { + LOGGER.debug("Recursively deleting stale directory: {}", stagingDir.relativize(dirToDelete)); + deleteDirectory(dirToDelete); + } + + // Try to delete staging directory itself if now empty + try { + Files.delete(stagingDir); + LOGGER.debug("Deleted empty staging directory: {}", stagingDir); + } catch (IOException e) { + LOGGER.debug("Staging directory not empty, preserving for other modules"); + } + + } catch (IOException e) { + LOGGER.warn("Failed to restore artifacts from staging directory: {}", e.getMessage()); + } + + // Clear the staging directory reference + state.stagingDirectory = null; + + // Remove the project state from map to free memory (called after save() cleanup) + String key = getVersionlessProjectKey(project); + projectStates.remove(key); + } + + /** + * Recursively delete a directory and all its contents. + */ + private void deleteDirectory(Path dir) throws IOException { + if (!Files.exists(dir)) { + return; + } + + Files.walkFileTree(dir, new SimpleFileVisitor() { + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + Files.delete(file); + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { + Files.delete(dir); + return FileVisitResult.CONTINUE; + } + }); + } + private boolean isOutputArtifact(String name) { List excludePatterns = cacheConfig.getExcludePatterns(); for (Pattern pattern : excludePatterns) { diff --git a/src/main/mdo/build-cache-build.mdo b/src/main/mdo/build-cache-build.mdo index 0e639352..0ac4c7f5 100644 --- a/src/main/mdo/build-cache-build.mdo +++ b/src/main/mdo/build-cache-build.mdo @@ -244,6 +244,12 @@ under the License. filePath String + + + isDirectory + boolean + Indicates if this artifact represents a directory (e.g., target/classes) that was zipped for caching + diff --git a/src/test/java/org/apache/maven/buildcache/its/GitCheckoutStaleArtifactTest.java b/src/test/java/org/apache/maven/buildcache/its/GitCheckoutStaleArtifactTest.java new file mode 100644 index 00000000..b19c6c4e --- /dev/null +++ b/src/test/java/org/apache/maven/buildcache/its/GitCheckoutStaleArtifactTest.java @@ -0,0 +1,83 @@ +/* + * 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.its; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.attribute.FileTime; +import java.time.Instant; +import java.util.Arrays; + +import org.apache.maven.buildcache.its.junit.IntegrationTest; +import org.apache.maven.it.VerificationException; +import org.apache.maven.it.Verifier; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Tests that stale artifacts from git branch switches are not cached. + * Simulates the scenario: + * 1. Build on branch A (creates target/classes with old content) + * 2. Git checkout branch B (sources change, but target/classes remains) + * 3. Build without 'mvn clean' - should NOT cache stale target/classes + */ +@IntegrationTest("src/test/projects/git-checkout-stale-artifact") +class GitCheckoutStaleArtifactTest { + + @Test + void staleDirectoryNotCached(Verifier verifier) throws VerificationException, IOException { + verifier.setAutoclean(false); + + // Simulate branch A: compile project + verifier.setLogFileName("../log-branch-a.txt"); + verifier.executeGoals(Arrays.asList("clean", "compile")); + verifier.verifyErrorFreeLog(); + + Path classesDir = Paths.get(verifier.getBasedir(), "target", "classes"); + Path appClass = classesDir.resolve("org/example/App.class"); + assertTrue(Files.exists(appClass), "App.class should exist after compile"); + + // Simulate git checkout to branch B by: + // 1. Modifying source file (simulates different branch content) + // 2. Making class file appear OLDER than build start time (stale) + Path sourceFile = Paths.get(verifier.getBasedir(), "src/main/java/org/example/App.java"); + String content = new String(Files.readAllBytes(sourceFile), "UTF-8"); + Files.write(sourceFile, content.replace("Branch A", "Branch B").getBytes("UTF-8")); + + // Backdate the class file to simulate stale artifact from previous branch + FileTime oldTime = FileTime.from(Instant.now().minusSeconds(3600)); // 1 hour ago + Files.setLastModifiedTime(appClass, oldTime); + + // Try to build without clean (simulates developer workflow) + verifier.setLogFileName("../log-branch-b.txt"); + verifier.executeGoals(Arrays.asList("compile")); + verifier.verifyErrorFreeLog(); + + // Verify that compiler detected source change and recompiled + // (class file should have new timestamp after recompile) + FileTime newTime = Files.getLastModifiedTime(appClass); + assertTrue( + newTime.toMillis() > oldTime.toMillis(), + "Compiler should have recompiled stale class (new timestamp: " + newTime + ", old timestamp: " + oldTime + + ")"); + } +} diff --git a/src/test/java/org/apache/maven/buildcache/its/GitCheckoutStaleMultimoduleTest.java b/src/test/java/org/apache/maven/buildcache/its/GitCheckoutStaleMultimoduleTest.java new file mode 100644 index 00000000..e38307d5 --- /dev/null +++ b/src/test/java/org/apache/maven/buildcache/its/GitCheckoutStaleMultimoduleTest.java @@ -0,0 +1,105 @@ +/* + * 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.its; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.attribute.FileTime; +import java.time.Instant; +import java.util.Arrays; + +import org.apache.maven.buildcache.its.junit.IntegrationTest; +import org.apache.maven.it.VerificationException; +import org.apache.maven.it.Verifier; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Tests that stale artifacts from git branch switches in multimodule projects are not cached. + * Verifies that the staging directory correctly preserves the full path structure including + * submodule paths relative to the multimodule root. + * + *

Scenario: + *

    + *
  1. Build multimodule project on branch A (creates module1/target/classes)
    2. + *
  2. Simulate git checkout to branch B (source changes, target/classes remains stale)
    3. + *
  3. Build without 'mvn clean' - should stage stale files with full path preservation
    4. + *
  4. Verify staging directory structure: target/.maven-build-cache-stash/module1/target/classes
    5. + *
+ */ +@IntegrationTest("src/test/projects/git-checkout-stale-multimodule") +class GitCheckoutStaleMultimoduleTest { + + @Test + void staleMultimoduleDirectoriesCorrectlyStaged(Verifier verifier) throws VerificationException, IOException { + verifier.setAutoclean(false); + + // Simulate branch A: compile multimodule project + verifier.setLogFileName("../log-multimodule-branch-a.txt"); + verifier.executeGoals(Arrays.asList("clean", "compile")); + verifier.verifyErrorFreeLog(); + + // Verify module1 class file was created + Path basedir = Paths.get(verifier.getBasedir()); + Path module1ClassesDir = basedir.resolve("module1/target/classes"); + Path module1Class = module1ClassesDir.resolve("org/example/Module1.class"); + assertTrue(Files.exists(module1Class), "Module1.class should exist after compile"); + + // Simulate git checkout to branch B by: + // 1. Modifying source file (simulates different branch content) + // 2. Making class file appear OLDER than build start time (stale) + Path sourceFile = basedir.resolve("module1/src/main/java/org/example/Module1.java"); + String content = new String(Files.readAllBytes(sourceFile), "UTF-8"); + Files.write(sourceFile, content.replace("Branch A", "Branch B").getBytes("UTF-8")); + + // Backdate the class file to simulate stale artifact from previous branch + FileTime oldTime = FileTime.from(Instant.now().minusSeconds(3600)); // 1 hour ago + Files.setLastModifiedTime(module1Class, oldTime); + + // Build without clean (simulates developer workflow) + // The staleness detection should: + // 1. Move module1/target/classes to target/.maven-build-cache-stash/module1/target/classes + // 2. Force recompilation (Maven sees clean module1/target/) + // 3. After save(), restore or discard based on whether files were rebuilt + verifier.setLogFileName("../log-multimodule-branch-b.txt"); + verifier.executeGoals(Arrays.asList("compile")); + verifier.verifyErrorFreeLog(); + + // Verify that compiler detected source change and recompiled + // (class file should have new timestamp after recompile) + FileTime newTime = Files.getLastModifiedTime(module1Class); + assertTrue( + newTime.toMillis() > oldTime.toMillis(), + "Compiler should have recompiled stale class (new timestamp: " + newTime + ", old timestamp: " + oldTime + + ")"); + + // Verify that staging directory was cleaned up after restore + // After a successful build, all files should be either: + // 1. Restored (moved back to original location) - for unchanged files + // 2. Discarded (deleted from staging) - for rebuilt files + // So the staging directory should be empty or deleted + Path stagingDir = basedir.resolve("target/maven-build-cache-extension"); + assertTrue( + !Files.exists(stagingDir), + "Staging directory should be deleted after all files are restored or discarded"); + } +} diff --git a/src/test/projects/git-checkout-stale-artifact/.mvn/maven-build-cache-config.xml b/src/test/projects/git-checkout-stale-artifact/.mvn/maven-build-cache-config.xml new file mode 100644 index 00000000..6dc0f129 --- /dev/null +++ b/src/test/projects/git-checkout-stale-artifact/.mvn/maven-build-cache-config.xml @@ -0,0 +1,12 @@ + + + + + + classes + + + + diff --git a/src/test/projects/git-checkout-stale-artifact/pom.xml b/src/test/projects/git-checkout-stale-artifact/pom.xml new file mode 100644 index 00000000..a4ce8dc6 --- /dev/null +++ b/src/test/projects/git-checkout-stale-artifact/pom.xml @@ -0,0 +1,26 @@ + + + + 4.0.0 + org.apache.maven.caching.test + git-checkout-stale-artifact + 0.0.1-SNAPSHOT + jar + + + 1.8 + 1.8 + + + + + + org.apache.maven.extensions + maven-build-cache-extension + ${projectVersion} + + + + + diff --git a/src/test/projects/git-checkout-stale-artifact/src/main/java/org/example/App.java b/src/test/projects/git-checkout-stale-artifact/src/main/java/org/example/App.java new file mode 100644 index 00000000..757987ff --- /dev/null +++ b/src/test/projects/git-checkout-stale-artifact/src/main/java/org/example/App.java @@ -0,0 +1,7 @@ +package org.example; + +public class App { + public static void main(String[] args) { + System.out.println("Branch A"); + } +} diff --git a/src/test/projects/git-checkout-stale-multimodule/.mvn/maven-build-cache-config.xml b/src/test/projects/git-checkout-stale-multimodule/.mvn/maven-build-cache-config.xml new file mode 100644 index 00000000..6dc0f129 --- /dev/null +++ b/src/test/projects/git-checkout-stale-multimodule/.mvn/maven-build-cache-config.xml @@ -0,0 +1,12 @@ + + + + + + classes + + + + diff --git a/src/test/projects/git-checkout-stale-multimodule/module1/pom.xml b/src/test/projects/git-checkout-stale-multimodule/module1/pom.xml new file mode 100644 index 00000000..329dd9af --- /dev/null +++ b/src/test/projects/git-checkout-stale-multimodule/module1/pom.xml @@ -0,0 +1,31 @@ + + + 4.0.0 + + + org.example + stale-multimodule-parent + 1.0-SNAPSHOT + + + module1 + diff --git a/src/test/projects/git-checkout-stale-multimodule/module1/src/main/java/org/example/Module1.java b/src/test/projects/git-checkout-stale-multimodule/module1/src/main/java/org/example/Module1.java new file mode 100644 index 00000000..ab8ef85d --- /dev/null +++ b/src/test/projects/git-checkout-stale-multimodule/module1/src/main/java/org/example/Module1.java @@ -0,0 +1,7 @@ +package org.example; + +public class Module1 { + public static void main(String[] args) { + System.out.println("Module1 Branch A"); + } +} diff --git a/src/test/projects/git-checkout-stale-multimodule/pom.xml b/src/test/projects/git-checkout-stale-multimodule/pom.xml new file mode 100644 index 00000000..4dd27ace --- /dev/null +++ b/src/test/projects/git-checkout-stale-multimodule/pom.xml @@ -0,0 +1,47 @@ + + + 4.0.0 + + org.example + stale-multimodule-parent + 1.0-SNAPSHOT + pom + + + 1.8 + 1.8 + + + + + + org.apache.maven.extensions + maven-build-cache-extension + ${projectVersion} + + + + + + module1 + + From a049102c306f322e0d4f61db9611a6a0f49e4fab Mon Sep 17 00:00:00 2001 From: cowwoc Date: Sun, 2 Nov 2025 16:08:59 -0500 Subject: [PATCH 6/6] Revert Spotless Maven Plugin upgrade Remove Spotless 3.0.0 upgrade and palantirJavaFormat configuration to keep the PR focused on compile-phase caching functionality. --- pom.xml | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/pom.xml b/pom.xml index cd7d325f..33ee638f 100644 --- a/pom.xml +++ b/pom.xml @@ -64,7 +64,6 @@ under the License. 3.9.0 11 2.6.0 - 3.0.0 4.0.0-alpha-8 @@ -428,17 +427,6 @@ under the License. - - com.diffplug.spotless - spotless-maven-plugin - - - - 2.81.0 - - - -