Save attached outputs for compile-only cache entries

[cowwoc]

Summary

This PR fixes incomplete cache restoration from compile-only builds by ensuring that configured outputs (e.g., classes, test-classes, module-info) are always attached and saved to the cache, regardless of the highest lifecycle phase executed.

Changes Made

1. Reset attached resource bookkeeping per save invocation

   • Clears attachedResourcesPathsById and resets attachedResourceCounter at the start of each save() call
   • Prevents stale artifacts from previous builds in multi-module projects from polluting cache entries

2. Attach outputs for all lifecycle phases

   • Moves attachGeneratedSources() and attachOutputs() outside the hasLifecyclePhase("package") conditional
   • Ensures compile-only builds persist configured output directories to enable cache hits in subsequent builds
   • Maintains consistency between save and restore operations: what gets cached can be restored

3. Add comprehensive integration test

   • Tests JPMS multi-module project with compile-only followed by verify
   • Verifies module-info.class files are restored correctly after cache hit
   • Covers the exact scenario reported in Incomplete Cache Restoration from compile-Only Builds #393

Addressing Reviewer Feedback from PR #176

In PR #176#discussion_r1732039886, @AlexanderAshitkin suggested:

"More accurate implementation could support pre-compile phases - 'save' and restore generated sources and continue with compile."

This PR implements exactly that suggestion:

• Pre-compile phase support: Compile-only builds now save all configured outputs
• Generated sources: Already supported via attachGeneratedSources()
• Continue with compile: Subsequent builds restore outputs and continue seamlessly with later phases
• Save/Restore consistency: Attached outputs are saved unconditionally and restored based on configuration (controlled by isRestoreGeneratedSources())

Root Cause Analysis

Before this PR:

if (project.hasLifecyclePhase("package")) {
    attachGeneratedSources(project);  // Only for package phase
    attachOutputs(project);           // Only for package phase
    // ...
}

Problem: Compile-only builds (mvn clean compile) never called attachOutputs(), creating cache entries without critical files like module-info.class.

After this PR:

attachGeneratedSources(project);  // Always called
attachOutputs(project);           // Always called

Result: Compile-only builds save configured outputs, enabling proper restoration in subsequent builds.

Testing

Run the integration test:

./mvnw -Prun-its -Dit.test=Issue393CompileRestoreTest verify

The test validates:

1. ✅ mvn clean compile creates cache entry with module-info.class
2. ✅ mvn clean verify restores from cache including module-info.class
3. ✅ Consumer module can compile against cached JPMS module descriptors
4. ✅ Tests execute successfully with restored artifacts

Related Issues

Fixes #393 - Incomplete Cache Restoration from compile-only Builds
Fixes #259 - Cannot restore cache after mvn clean commands
Fixes #340 - Cache fails to restore compiled classes with mvn clean test

Migration Notes

No configuration changes required. Existing projects will automatically benefit from this fix on their next build.

---

Note on Implementation Philosophy: This PR prioritizes correctness and consistency over optimization. All lifecycle phases save configured outputs to ensure cache integrity. Future optimizations could selectively skip certain outputs based on phase, but this would require careful analysis to avoid reintroducing the bugs fixed here.

[cowwoc]

Updates Based on Reviewer Feedback

I've improved this PR to address the concerns raised by @AlexanderAshitkin in PR #176's review:

Code Improvements

1. Added detailed inline comments explaining:

   • Why bookkeeping reset is necessary (prevents stale artifacts in multi-module builds)
   • That outputs are attached for ALL lifecycle phases (including compile-only)
   • How this addresses the JPMS module descriptor restoration issues

2. Added debug logging to make compile-only cache saves visible in build logs

3. Enhanced PR description with:

   • Clear explanation of how this implements the "pre-compile phase" suggestion
   • Root cause analysis comparing before/after behavior
   • Explicit mapping to the reviewer's feedback

Addressing "Save/Restore Consistency"

The reviewer's concern about consistency between save and restore operations is addressed:

Save side (this PR):

• Always attaches configured outputs (classes, test-classes, etc.)
• Creates cache entries that contain all necessary artifacts for restoration

Restore side (existing code):

• Restores attached outputs based on configuration (isRestoreGeneratedSources())
• The configuration controls what gets restored, not what gets saved

This design ensures:

• ✅ Compile-only builds can be restored by subsequent builds
• ✅ Users retain control over restoration behavior via configuration
• ✅ No cache entries are created without necessary outputs

Testing

The integration test validates the complete workflow:

./mvnw -Prun-its -Dit.test=Issue393CompileRestoreTest verify

Test coverage includes:

• JPMS module descriptors (module-info.class)
• Multi-module projects with inter-module dependencies
• Compile-only → verify workflow (the reported bug scenario)

[AlexanderAshitkin]

Conditioning this logic on package served two main purposes:

1. It prevented premature caching of outputs if the corresponding lifecycles have not run. Doing otherwise could lead to costly and incorrect caching or restoration of the previous branch's compilation output. Here’s a problematic sequence:

   • Compile Branch A and then checkout Branch B.
   • Run process-resources in Branch B. Result: the compiled sources of Branch A are cached, under checksum for Branch B.
   • Run the compilation in Branch B. Result: the compiled classes from Branch A are restored in Branch B, potentially interfering with the compilation of Branch B.
     Although such cached entries can be replaced later, it is still not desirable.

2. Conditioning on package also reduces the number of caching and unpacking operations. Specifically, it avoids the need to zip, download, or upload files during every compilation, which helps maintain performance. When an engineer is actively working on the code, is it really beneficial to cache intermediate results? It is challenging to estimate how useful this would be in practice, and I assume it won’t always be desirable.

Just thinking aloud, from the user's perspective, intentionally invoking restoration seems to offer better usability. In that sense, Gradle cache model is also flawed - saving a zip of classes with every compile task seems excessive and results in the io/cpu spent on creation of unnecessary cache entries that contain intermediate compilation results. In that sense package is not that bad - it allows to throttle costly operation and run it together with other costly IO work.

[cowwoc]

Ah! That makes much more sense. Thank you for clarifying why this logic was conditioned no the package phase.

To tackle this problem, I've combined timestamp-based filtering with per-project thread-safe isolation to prevent both the branch-switching scenario and race conditions in multi-threaded builds.

How It Works

1. Timestamp Filtering

The cache now only captures outputs that were either:

• Modified during the current build (timestamp >= buildStartTime), OR
• Restored from cache during this build (tracked explicitly)

This elegantly handles your branch-switching scenario:

1. mvn compile on Branch A at 10:00:00 → 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: lastModified (10:00:01) < buildStartTime (10:01:00)
   AND not restored this build → SKIPPED ✓

2. Handling Cache Hits

Your question about mvn package with a compile cache hit is crucial. The solution tracks restored outputs:

1. First build: mvn compile at 10:00:00

• Compiles classes, caches them ✓

2. Second build: mvn package at 10:05:00

• Compile phase: Cache hit, restores to target/classes
• Restoration tracked: classifier added to state.restoredOutputClassifiers
• Package phase save() checks:
  • Fresh package outputs: timestamp >= 10:05:00 → include ✓
  • Restored compile outputs: in restoredOutputClassifiers → include ✓
  • Stale old outputs: timestamp < 10:05:00 AND not restored → exclude ✓

3. Thread Safety for Multi-Threaded Builds

Per your comment about multi-threaded builds, I've also fixed the existing thread safety issues:

Problem: CacheControllerImpl is @SessionScoped (one instance shared across all modules). The original code used:

private final Map<String, Path> attachedResourcesPathsById = new HashMap<>();
private int attachedResourceCounter = 0;

With mvn -T 4, calling clear() in one thread would affect other threads' modules.

Solution: Per-project isolation using ConcurrentHashMap:

private static class ProjectCacheState {
    final Map<String, Path> attachedResourcesPathsById = new HashMap<>();
    int attachedResourceCounter = 0;
    final Set<String> restoredOutputClassifiers = new HashSet<>();
}

private final ConcurrentMap<String, ProjectCacheState> projectStates = new ConcurrentHashMap<>();

Each module gets isolated state. Cleanup happens per-project in save()'s finally block.

Implementation Details

In CacheControllerImpl.java:

1. Capture build start time:
   final long buildStartTime = session.getRequest().getStartTime().getTime();
2. Track restored outputs in restoreProjectArtifacts():
   state.restoredOutputClassifiers.add(attachedArtifactInfo.getClassifier());
3. Check timestamps in attachDirIfNotEmpty():

long lastModified = Files.getLastModifiedTime(candidateSubDir).toMillis();
boolean isRestoredThisBuild = state.restoredOutputClassifiers.contains(classifier);

if (lastModified < buildStartTime && !isRestoredThisBuild) {
    // Skip stale outputs
    return;
}

Testing

All existing tests pass, and the approach has been validated through:

• Compilation verification
• Full test suite execution
• Thread safety analysis of concurrent access patterns

Let me know if you have any concerns about this approach or if you'd like me to add additional safeguards or test cases.

[cowwoc]

Addressing Performance Concerns

Thank you for raising the important question about performance overhead during active development. You're absolutely right that caching compile outputs on every mvn compile has costs:

• IO overhead: Zipping classes directories
• CPU overhead: Computing hashes
• Network overhead: Potentially uploading to remote cache

Solution: Configurable Compile Caching

I've added a new property maven.build.cache.cacheCompile (default: true) to give users control:

# Disable compile caching during active development:
mvn clean compile -Dmaven.build.cache.cacheCompile=false

# Enable for CI/multi-module scenarios (default):
mvn clean compile

Design Rationale

Default: true - Maintains the fix for issue #393 without breaking changes. Users experiencing compile-only cache restoration issues get the fix automatically.

Opt-out available - Developers actively editing code can disable compile caching to avoid overhead, while still benefiting from package-phase caching.

Use Cases

When to keep enabled (default):

• Large multi-module projects (editing 1 of 50 modules)
• CI/CD pipelines for quick feedback
• Branch switching scenarios
• Working on test code only

When to disable:

• Actively editing a single module
• Rapid edit-compile-test cycles
• Development environments where package phase is always run anyway

Implementation

The property gates the calls to attachGeneratedSources() and attachOutputs() in CacheControllerImpl.save():

if (cacheConfig.isCacheCompile()) {
    attachGeneratedSources(project, state, buildStartTime);
    attachOutputs(project, state, buildStartTime);
}

This means:

• When false: mvn compile creates NO cache entry (no zipping, no IO)
• When true (default): Full caching as implemented for issue Incomplete Cache Restoration from compile-Only Builds #393

Testing

Added CacheCompileDisabledTest to verify:

• ✅ With property disabled: No cache entries created, no restoration on second compile
• ✅ With property enabled: Cache entries created, restoration works on second compile

Ready for your review!

[olamy]

If I understand correctly, all of this is for being able to use the cache with mvn compile? Why not test-compile?

[elharo]

need to run mvn spotless:apply

[cowwoc]

@elharo Fixed. Try now.
@olamy The code change should work for both compile and test-compile. Did I misunderstand your question?

[cowwoc]

I fixed the build failures. At the same time, I introduced a change that requires your review.

To protect against caching stale artifacts caused by:

1. Future timestamps (due to clock skew or malicious parties)
2. Orphaned class files (from deleted source files)
3. Stale JARs/WARs (from previous builds)

I implemented a "staging directory" approach. The performance impact should be minimal as the staging operation only occurs for uncached builds and uses fast filesystem moves rather than copies. Cached builds are unaffected.

How it works

Before the build: Pre-existing artifacts are moved to target/maven-build-cache-extension/ in the multimodule root, preserving their full path structure (e.g., module1/target/classes). This ensures Maven sees a clean target directory and must rebuild everything.

After save(): Files are restored from staging if they weren't rebuilt (indicating they're still valid), or discarded if fresh versions exist.

This approach is more robust than timestamp comparison, as it physically separates potentially stale artifacts rather than relying on heuristics.

[cowwoc]

Thanks @erik-meuwese-topicus! Fixed.

Can you please rerun the build?

[erik-meuwese-topicus]

This pull request still has a needs work. The review comments have been resolved and the build succes Are there still things left that need to be discussed or changed before this pull request can get approved? @olamy @elharo

[elharo]

Hmm, is this whole cache system assuming git? Does it it work if a different source management system is used?

addressed

[cowwoc]

@elharo As far as I can tell, nothing in the PR's implementation is git-specific but I've committed an update that removes any git-specific terminology. Please let me know if I've missed anything. Thanks!

[Copilot]

Pull request overview

This PR fixes incomplete cache restoration from compile-only builds by ensuring that configured outputs (e.g., classes, test-classes, module-info) are always attached and saved to the cache. The key change removes the conditional that previously limited output attachment to the package phase, enabling compile-only builds to create restorable cache entries.

• Introduces per-project cache state management for thread safety
• Implements staging directory mechanism to prevent caching stale artifacts
• Adds maven.build.cache.cacheCompile flag to control compile-phase caching

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java	Core changes to save logic, artifact staging/restoration, and state management
src/main/java/org/apache/maven/buildcache/xml/CacheConfig.java	Adds isCacheCompile() method to configuration interface
src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java	Implements CACHE_COMPILE property with default true
src/main/java/org/apache/maven/buildcache/CacheController.java	Adds interface methods for staging/restoring artifacts
src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java	Integrates artifact staging into build execution flow
src/main/mdo/build-cache-build.mdo	Adds isDirectory field to artifact metadata
src/test/java/org/apache/maven/buildcache/its/Issue393CompileRestoreTest.java	Integration test validating compile-only cache restoration
src/test/java/org/apache/maven/buildcache/its/CacheCompileDisabledTest.java	Tests behavior when cacheCompile flag is disabled
src/test/java/org/apache/maven/buildcache/its/StaleArtifactTest.java	Tests prevention of stale artifact caching
src/test/java/org/apache/maven/buildcache/its/StaleMultimoduleArtifactTest.java	Tests stale artifact handling in multi-module projects
src/test/java/org/apache/maven/buildcache/its/MandatoryCleanTest.java	Adds debug logging to existing test
src/test/java/org/apache/maven/buildcache/its/BuildExtensionTest.java	Adds debug logging to existing test
src/test/projects/issue-393-compile-restore/*	Test project files for Issue 393 integration test
src/test/projects/stale-artifact/*	Test project files for stale artifact test
src/test/projects/stale-multimodule-artifact/*	Test project files for stale multimodule artifact test

Comments suppressed due to low confidence (1)

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

• The error message 'Cannot restore file' should include the restoration path to help diagnose issues. Add restorationPath to the error message for better debugging context.

        if (cacheConfig.isRestoreOnDiskArtifacts() && MavenProjectInput.isRestoreOnDiskArtifacts(project)) {
            Path restorationPath = project.getBasedir().toPath().resolve(artifact.getFilePath());
            final AtomicBoolean restored = new AtomicBoolean(false);
            return file -> {
                // Set to restored even if it fails later, we don't want multiple try
                if (restored.compareAndSet(false, true)) {
                    verifyRestorationInsideProject(project, restorationPath);
                    try {
                        restoreArtifactToDisk(file, artifact, restorationPath);
                    } catch (IOException e) {
                        LOGGER.error("Cannot restore file " + artifact.getFileName(), e);
                        throw new RuntimeException(e);
                    }
                }
                return restorationPath.toFile();
            };
        }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[cowwoc]

This is a never-ending review cycle. What will it take to get this PR merged ...?

[Bukama]

The implementation looks valuable, but I only have limited knowledge about the BCE. But what I recognize that no documentation has been update - even with the fact that there is a new configuration parameter.

[cowwoc]

@olamy Can you please investigate the latest build failure? It seems to be unrelated to this PR. I see:

java.io.FileNotFoundException: /Users/runner/.m2/settings-security.xml (No such file or directory)

and

org.apache.http.client.HttpResponseException: status code: 500, reason phrase: Internal Server Error (500)

[cowwoc]

Thank you for the review @Bukama! You're right - I've added documentation for the new maven.build.cache.cacheCompile parameter:

• parameters.md: Added to the command line flags table with description and usage scenario
• how-to.md: Added a new section "I want to disable caching of compile-only builds" explaining when users might want to disable this feature

See commit e3d8e58.

[cowwoc]

@Bukama Good catch. Fixed by 6a65421

[cowwoc]

Thanks @olamy!

[guylabs]

@cowwoc Running a build with these changes and -Dmaven.build.cache.enabled=false set fails the build and prints this exception:

[ERROR] Cache is not initialized. Actual state: DISABLED
java.lang.IllegalStateException: Cache is not initialized. Actual state: DISABLED
    at org.apache.maven.buildcache.xml.CacheConfigImpl.checkInitializedState(CacheConfigImpl.java:648)
    at org.apache.maven.buildcache.xml.CacheConfigImpl.getAttachedOutputs(CacheConfigImpl.java:582)
    at org.apache.maven.buildcache.CacheControllerImpl.collectCachedArtifactPaths(CacheControllerImpl.java:1315)
    at org.apache.maven.buildcache.CacheControllerImpl.stagePreExistingArtifacts(CacheControllerImpl.java:1249)
    at org.apache.maven.buildcache.BuildCacheMojosExecutionStrategy.execute(BuildCacheMojosExecutionStrategy.java:161)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:165)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:110)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:76)
    at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build(SingleThreadedBuilder.java:61)
    at org.apache.maven.lifecycle.internal.DefaultLifecycleStarter.execute(DefaultLifecycleStarter.java:123)
    at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:310)
    at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:225)
    at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:149)
    at org.apache.maven.cling.invoker.mvn.MavenInvoker.doExecute(MavenInvoker.java:452)
    at org.apache.maven.cling.invoker.mvn.MavenInvoker.execute(MavenInvoker.java:97)
    at org.apache.maven.cling.invoker.mvn.MavenInvoker.execute(MavenInvoker.java:81)
    at org.apache.maven.cling.invoker.LookupInvoker.doInvoke(LookupInvoker.java:165)
    at org.apache.maven.cling.invoker.LookupInvoker.invoke(LookupInvoker.java:134)
    at org.apache.maven.cling.ClingSupport.run(ClingSupport.java:76)
    at org.apache.maven.cling.MavenCling.main(MavenCling.java:64)
    at org.codehaus.groovy.vmplugin.v8.IndyInterface.selectMethod(IndyInterface.java:355)
    at org.codehaus.groovy.vmplugin.v8.IndyInterface.fromCache(IndyInterface.java:321)
    at com.gradle.testlib.use.runner.Embedded3xLauncher.doMain(Embedded3xLauncher.groovy:315)
    at com.gradle.testlib.use.runner.Embedded3xLauncher$_run_closure3.doCall(Embedded3xLauncher.groovy:282)
    at com.gradle.testlib.use.runner.Embedded3xLauncher$_run_closure3.call(Embedded3xLauncher.groovy)
    at java.util.concurrent.FutureTask.run(FutureTask.java:317)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
    at java.lang.Thread.run(Thread.java:1583)
[ERROR] 

Running it together with -Dmaven.build.cache.cacheCompile=false will not fail the build.

Would it be possible to include this use case as well, as this is an official configuration option? Thanks!

[cowwoc]

@guylabs Good catch! I've created #424 to fix this bug. Thanks for the report.