Skip to content

Commit 92822da

Browse files
committed
Add timestamp filtering and per-project thread-safe isolation
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<String, ProjectCacheState> 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 ✓ ```
1 parent 19f4a0a commit 92822da

File tree

1 file changed

+66
-25
lines changed

1 file changed

+66
-25
lines changed

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

Lines changed: 66 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import java.util.ArrayList;
4141
import java.util.Collections;
4242
import java.util.HashMap;
43+
import java.util.HashSet;
4344
import java.util.List;
4445
import java.util.Map;
4546
import java.util.Objects;
@@ -135,13 +136,24 @@ public class CacheControllerImpl implements CacheController {
135136
private volatile Scm scm;
136137

137138
/**
138-
* A map dedicated to store the base path of resources stored to the cache which are not original artifacts
139-
* (ex : generated source basedir).
140-
* Used to link the resource to its path on disk
139+
* Per-project cache state to ensure thread safety in multi-threaded builds.
140+
* Each project gets isolated state for resource tracking, counters, and restored output tracking.
141141
*/
142-
private final Map<String, Path> attachedResourcesPathsById = new HashMap<>();
142+
private static class ProjectCacheState {
143+
final Map<String, Path> attachedResourcesPathsById = new HashMap<>();
144+
int attachedResourceCounter = 0;
145+
final Set<String> restoredOutputClassifiers = new HashSet<>();
146+
}
147+
148+
private final ConcurrentMap<String, ProjectCacheState> projectStates = new ConcurrentHashMap<>();
143149

144-
private int attachedResourceCounter = 0;
150+
/**
151+
* Get or create cache state for the given project (thread-safe).
152+
*/
153+
private ProjectCacheState getProjectState(MavenProject project) {
154+
String key = getVersionlessProjectKey(project);
155+
return projectStates.computeIfAbsent(key, k -> new ProjectCacheState());
156+
}
145157
// CHECKSTYLE_OFF: ParameterNumber
146158
@Inject
147159
public CacheControllerImpl(
@@ -356,6 +368,7 @@ public ArtifactRestorationReport restoreProjectArtifacts(CacheResult cacheResult
356368
final Build build = cacheResult.getBuildInfo();
357369
final CacheContext context = cacheResult.getContext();
358370
final MavenProject project = context.getProject();
371+
final ProjectCacheState state = getProjectState(project);
359372
ArtifactRestorationReport restorationReport = new ArtifactRestorationReport();
360373

361374
try {
@@ -397,6 +410,8 @@ public ArtifactRestorationReport restoreProjectArtifacts(CacheResult cacheResult
397410
final Path attachedArtifactFile =
398411
localCache.getArtifactFile(context, cacheResult.getSource(), attachedArtifactInfo);
399412
restoreGeneratedSources(attachedArtifactInfo, attachedArtifactFile, project);
413+
// Track this classifier as restored so save() includes it even with old timestamp
414+
state.restoredOutputClassifiers.add(attachedArtifactInfo.getClassifier());
400415
}
401416
} else {
402417
Future<File> downloadTask = createDownloadTask(
@@ -497,23 +512,27 @@ public void save(
497512

498513
final MavenProject project = context.getProject();
499514
final MavenSession session = context.getSession();
515+
final ProjectCacheState state = getProjectState(project);
500516
try {
501-
attachedResourcesPathsById.clear();
502-
attachedResourceCounter = 0;
517+
state.attachedResourcesPathsById.clear();
518+
state.attachedResourceCounter = 0;
519+
520+
// Get build start time to filter out stale artifacts from previous builds
521+
final long buildStartTime = session.getRequest().getStartTime().getTime();
503522

504523
final HashFactory hashFactory = cacheConfig.getHashFactory();
505524
final HashAlgorithm algorithm = hashFactory.createAlgorithm();
506525
final org.apache.maven.artifact.Artifact projectArtifact = project.getArtifact();
507526
final boolean hasPackagePhase = project.hasLifecyclePhase("package");
508527

509-
attachGeneratedSources(project);
510-
attachOutputs(project);
528+
attachGeneratedSources(project, state, buildStartTime);
529+
attachOutputs(project, state, buildStartTime);
511530

512531
final List<org.apache.maven.artifact.Artifact> attachedArtifacts = project.getAttachedArtifacts() != null
513532
? project.getAttachedArtifacts()
514533
: Collections.emptyList();
515-
final List<Artifact> attachedArtifactDtos = artifactDtos(attachedArtifacts, algorithm, project);
516-
final Artifact projectArtifactDto = hasPackagePhase ? artifactDto(project.getArtifact(), algorithm, project)
534+
final List<Artifact> attachedArtifactDtos = artifactDtos(attachedArtifacts, algorithm, project, state);
535+
final Artifact projectArtifactDto = hasPackagePhase ? artifactDto(project.getArtifact(), algorithm, project, state)
517536
: null;
518537

519538
List<CompletedExecution> completedExecution = buildExecutionInfo(mojoExecutions, executionEvents);
@@ -562,6 +581,10 @@ public void save(
562581
} catch (Exception ex) {
563582
LOGGER.error("Failed to clean cache due to unexpected error:", ex);
564583
}
584+
} finally {
585+
// Cleanup project state to free memory (thread-safe removal)
586+
String key = getVersionlessProjectKey(project);
587+
projectStates.remove(key);
565588
}
566589
}
567590

@@ -619,20 +642,22 @@ public void produceDiffReport(CacheResult cacheResult, Build build) {
619642
}
620643

621644
private List<Artifact> artifactDtos(
622-
List<org.apache.maven.artifact.Artifact> attachedArtifacts, HashAlgorithm digest, MavenProject project)
645+
List<org.apache.maven.artifact.Artifact> attachedArtifacts, HashAlgorithm digest, MavenProject project,
646+
ProjectCacheState state)
623647
throws IOException {
624648
List<Artifact> result = new ArrayList<>();
625649
for (org.apache.maven.artifact.Artifact attachedArtifact : attachedArtifacts) {
626650
if (attachedArtifact.getFile() != null
627651
&& isOutputArtifact(attachedArtifact.getFile().getName())) {
628-
result.add(artifactDto(attachedArtifact, digest, project));
652+
result.add(artifactDto(attachedArtifact, digest, project, state));
629653
}
630654
}
631655
return result;
632656
}
633657

634658
private Artifact artifactDto(
635-
org.apache.maven.artifact.Artifact projectArtifact, HashAlgorithm algorithm, MavenProject project)
659+
org.apache.maven.artifact.Artifact projectArtifact, HashAlgorithm algorithm, MavenProject project,
660+
ProjectCacheState state)
636661
throws IOException {
637662
final Artifact dto = DtoUtils.createDto(projectArtifact);
638663
if (projectArtifact.getFile() != null && projectArtifact.getFile().isFile()) {
@@ -641,7 +666,7 @@ private Artifact artifactDto(
641666
dto.setFileSize(Files.size(file));
642667

643668
// Get the relative path of any extra zip directory added to the cache
644-
Path relativePath = attachedResourcesPathsById.get(projectArtifact.getClassifier());
669+
Path relativePath = state.attachedResourcesPathsById.get(projectArtifact.getClassifier());
645670
if (relativePath == null) {
646671
// If the path was not a member of this map, we are in presence of an original artifact.
647672
// we get its location on the disk
@@ -895,15 +920,15 @@ private void restoreGeneratedSources(Artifact artifact, Path artifactFilePath, M
895920
}
896921

897922
// TODO: move to config
898-
public void attachGeneratedSources(MavenProject project) throws IOException {
923+
public void attachGeneratedSources(MavenProject project, ProjectCacheState state, long buildStartTime) throws IOException {
899924
final Path targetDir = Paths.get(project.getBuild().getDirectory());
900925

901926
final Path generatedSourcesDir = targetDir.resolve("generated-sources");
902-
attachDirIfNotEmpty(generatedSourcesDir, targetDir, project, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB);
927+
attachDirIfNotEmpty(generatedSourcesDir, targetDir, project, state, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB, buildStartTime);
903928

904929
final Path generatedTestSourcesDir = targetDir.resolve("generated-test-sources");
905930
attachDirIfNotEmpty(
906-
generatedTestSourcesDir, targetDir, project, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB);
931+
generatedTestSourcesDir, targetDir, project, state, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB, buildStartTime);
907932

908933
Set<String> sourceRoots = new TreeSet<>();
909934
if (project.getCompileSourceRoots() != null) {
@@ -919,18 +944,18 @@ public void attachGeneratedSources(MavenProject project) throws IOException {
919944
&& sourceRootPath.startsWith(targetDir)
920945
&& !(sourceRootPath.startsWith(generatedSourcesDir)
921946
|| sourceRootPath.startsWith(generatedTestSourcesDir))) { // dir within target
922-
attachDirIfNotEmpty(sourceRootPath, targetDir, project, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB);
947+
attachDirIfNotEmpty(sourceRootPath, targetDir, project, state, OutputType.GENERATED_SOURCE, DEFAULT_FILE_GLOB, buildStartTime);
923948
}
924949
}
925950
}
926951

927-
private void attachOutputs(MavenProject project) throws IOException {
952+
private void attachOutputs(MavenProject project, ProjectCacheState state, long buildStartTime) throws IOException {
928953
final List<DirName> attachedDirs = cacheConfig.getAttachedOutputs();
929954
for (DirName dir : attachedDirs) {
930955
final Path targetDir = Paths.get(project.getBuild().getDirectory());
931956
final Path outputDir = targetDir.resolve(dir.getValue());
932957
if (isPathInsideProject(project, outputDir)) {
933-
attachDirIfNotEmpty(outputDir, targetDir, project, OutputType.EXTRA_OUTPUT, dir.getGlob());
958+
attachDirIfNotEmpty(outputDir, targetDir, project, state, OutputType.EXTRA_OUTPUT, dir.getGlob(), buildStartTime);
934959
} else {
935960
LOGGER.warn("Outside project output candidate directory discarded ({})", outputDir.normalize());
936961
}
@@ -941,16 +966,32 @@ private void attachDirIfNotEmpty(
941966
Path candidateSubDir,
942967
Path parentDir,
943968
MavenProject project,
969+
ProjectCacheState state,
944970
final OutputType attachedOutputType,
945-
final String glob)
971+
final String glob,
972+
final long buildStartTime)
946973
throws IOException {
947974
if (Files.isDirectory(candidateSubDir) && hasFiles(candidateSubDir)) {
948975
final Path relativePath = project.getBasedir().toPath().relativize(candidateSubDir);
949-
attachedResourceCounter++;
950-
final String classifier = attachedOutputType.getClassifierPrefix() + attachedResourceCounter;
976+
state.attachedResourceCounter++;
977+
final String classifier = attachedOutputType.getClassifierPrefix() + state.attachedResourceCounter;
978+
979+
// Check if directory was modified during this build OR was restored from cache
980+
long lastModified = Files.getLastModifiedTime(candidateSubDir).toMillis();
981+
boolean isRestoredThisBuild = state.restoredOutputClassifiers.contains(classifier);
982+
983+
if (lastModified < buildStartTime && !isRestoredThisBuild) {
984+
LOGGER.debug(
985+
"Skipping stale directory: {} (modified at {}, build started at {}, not restored)",
986+
candidateSubDir,
987+
lastModified,
988+
buildStartTime);
989+
return;
990+
}
991+
951992
boolean success = zipAndAttachArtifact(project, candidateSubDir, classifier, glob);
952993
if (success) {
953-
attachedResourcesPathsById.put(classifier, relativePath);
994+
state.attachedResourcesPathsById.put(classifier, relativePath);
954995
LOGGER.debug("Attached directory: {}", candidateSubDir);
955996
}
956997
}

0 commit comments

Comments
 (0)