Skip to content

Commit 95705f4

Browse files
committed
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
1 parent 9e86503 commit 95705f4

File tree

13 files changed

+820
-51
lines changed

13 files changed

+820
-51
lines changed

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

Lines changed: 44 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import javax.inject.Named;
2424

2525
import java.io.File;
26+
import java.io.IOException;
2627
import java.nio.file.Path;
2728
import java.util.HashSet;
2829
import java.util.List;
@@ -138,33 +139,58 @@ public void execute(
138139

139140
boolean restorable = result.isSuccess() || result.isPartialSuccess();
140141
boolean restored = false; // if partially restored need to save increment
142+
141143
if (restorable) {
142144
CacheRestorationStatus cacheRestorationStatus =
143145
restoreProject(result, mojoExecutions, mojoExecutionRunner, cacheConfig);
144146
restored = CacheRestorationStatus.SUCCESS == cacheRestorationStatus;
145147
executeExtraCleanPhaseIfNeeded(cacheRestorationStatus, cleanPhase, mojoExecutionRunner);
146148
}
147-
if (!restored) {
148-
for (MojoExecution mojoExecution : mojoExecutions) {
149-
if (source == Source.CLI
150-
|| mojoExecution.getLifecyclePhase() == null
151-
|| lifecyclePhasesHelper.isLaterPhaseThanClean(mojoExecution.getLifecyclePhase())) {
152-
mojoExecutionRunner.run(mojoExecution);
149+
150+
try {
151+
if (!restored && !forkedExecution) {
152+
// Move pre-existing artifacts to staging directory to prevent caching stale files
153+
// from previous builds (e.g., after git branch switch, or from cache restored
154+
// with clock skew). This ensures save() only sees fresh files built during this session.
155+
// Skip for forked executions since they don't cache and shouldn't modify artifacts.
156+
try {
157+
cacheController.stagePreExistingArtifacts(session, project);
158+
} catch (IOException e) {
159+
LOGGER.warn("Failed to stage pre-existing artifacts: {}", e.getMessage());
160+
// Continue build - if staging fails, we'll just cache what exists
153161
}
154162
}
155-
}
156163

157-
if (cacheState == INITIALIZED && (!result.isSuccess() || !restored)) {
158-
if (cacheConfig.isSkipSave()) {
159-
LOGGER.info("Cache saving is disabled.");
160-
} else if (cacheConfig.isMandatoryClean()
161-
&& lifecyclePhasesHelper
162-
.getCleanSegment(project, mojoExecutions)
163-
.isEmpty()) {
164-
LOGGER.info("Cache storing is skipped since there was no \"clean\" phase.");
165-
} else {
166-
final Map<String, MojoExecutionEvent> executionEvents = mojoListener.getProjectExecutions(project);
167-
cacheController.save(result, mojoExecutions, executionEvents);
164+
if (!restored) {
165+
for (MojoExecution mojoExecution : mojoExecutions) {
166+
if (source == Source.CLI
167+
|| mojoExecution.getLifecyclePhase() == null
168+
|| lifecyclePhasesHelper.isLaterPhaseThanClean(mojoExecution.getLifecyclePhase())) {
169+
mojoExecutionRunner.run(mojoExecution);
170+
}
171+
}
172+
}
173+
174+
if (cacheState == INITIALIZED && (!result.isSuccess() || !restored)) {
175+
if (cacheConfig.isSkipSave()) {
176+
LOGGER.info("Cache saving is disabled.");
177+
} else if (cacheConfig.isMandatoryClean()
178+
&& lifecyclePhasesHelper
179+
.getCleanSegment(project, mojoExecutions)
180+
.isEmpty()) {
181+
LOGGER.info("Cache storing is skipped since there was no \"clean\" phase.");
182+
} else {
183+
final Map<String, MojoExecutionEvent> executionEvents =
184+
mojoListener.getProjectExecutions(project);
185+
cacheController.save(result, mojoExecutions, executionEvents);
186+
}
187+
}
188+
} finally {
189+
// Always restore staged files after build completes (whether save ran or not).
190+
// Files that were rebuilt are discarded; files that weren't rebuilt are restored.
191+
// Skip for forked executions since they don't stage artifacts.
192+
if (!restored && !forkedExecution) {
193+
cacheController.restoreStagedArtifacts(session, project);
168194
}
169195
}
170196

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
*/
1919
package org.apache.maven.buildcache;
2020

21+
import java.io.IOException;
2122
import java.util.List;
2223
import java.util.Map;
2324

@@ -45,4 +46,23 @@ void save(
4546
boolean isForcedExecution(MavenProject project, MojoExecution execution);
4647

4748
void saveCacheReport(MavenSession session);
49+
50+
/**
51+
* Move pre-existing artifacts to staging directory to prevent caching stale files.
52+
* Called before mojos run to ensure save() only sees fresh files.
53+
*
54+
* @param session the Maven session
55+
* @param project the Maven project
56+
* @throws IOException if file operations fail
57+
*/
58+
void stagePreExistingArtifacts(MavenSession session, MavenProject project) throws IOException;
59+
60+
/**
61+
* Restore staged artifacts after save() completes.
62+
* Files that were rebuilt are discarded; files that weren't rebuilt are restored.
63+
*
64+
* @param session the Maven session
65+
* @param project the Maven project
66+
*/
67+
void restoreStagedArtifacts(MavenSession session, MavenProject project);
4868
}

0 commit comments

Comments
 (0)