-
Couldn't load subscription status.
- Fork 55
Fix timestamp preservation when extracting cached files #387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
10eb2c4 to
91e8b9c
Compare
how do you have this error? |
|
feedback provided in #388 |
Addresses feedback from PR apache#388 review comments and adds comprehensive unit tests to validate timestamp preservation through cache operations. Changes to CacheUtils.java: 1. Implement glob filtering for directory entries - Track directories containing matching files during visitFile() - Only add directory entries in postVisitDirectory() if they contain matching files or no glob filter is active - Preserves glob pattern contract while maintaining timestamp preservation 2. Fix Files.exists() race condition (TOCTOU vulnerability) - Remove unnecessary Files.exists() checks before Files.createDirectories() - Leverage idempotent behavior of createDirectories() - Eliminates time-of-check-to-time-of-use race condition 3. Add error handling for timestamp operations - Wrap Files.setLastModifiedTime() in try-catch blocks - Best-effort timestamp setting with graceful degradation - Prevents failures on filesystems without timestamp support New Unit Tests (CacheUtilsTimestampTest.java): - testFileTimestampPreservation: Verifies file timestamps preserved - testDirectoryTimestampPreservation: Verifies directory timestamps preserved - testDirectoryEntriesStoredInZip: Verifies directories stored in zip - testTimestampsInZipEntries: Verifies zip entry timestamps correct - testMavenWarningScenario: Reproduces Maven warning about file timestamps - testMultipleFilesTimestampConsistency: Verifies consistent timestamps Test Results: - All 6 tests pass with fixes applied - Tests are Java 8 compatible using helper methods - Clear failure messages with timestamp diffs for debugging Fixes apache#387 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
0b0ea6e to
91e8b9c
Compare
Addresses feedback from PR apache#388 review comments and adds comprehensive unit tests to validate timestamp preservation through cache operations. Changes to CacheUtils.java: 1. Implement glob filtering for directory entries - Track directories containing matching files during visitFile() - Only add directory entries in postVisitDirectory() if they contain matching files or no glob filter is active - Preserves glob pattern contract while maintaining timestamp preservation 2. Fix Files.exists() race condition (TOCTOU vulnerability) - Remove unnecessary Files.exists() checks before Files.createDirectories() - Leverage idempotent behavior of createDirectories() - Eliminates time-of-check-to-time-of-use race condition 3. Add error handling for timestamp operations - Wrap Files.setLastModifiedTime() in try-catch blocks - Best-effort timestamp setting with graceful degradation - Prevents failures on filesystems without timestamp support New Unit Tests (CacheUtilsTimestampTest.java): - testFileTimestampPreservation: Verifies file timestamps preserved - testDirectoryTimestampPreservation: Verifies directory timestamps preserved - testDirectoryEntriesStoredInZip: Verifies directories stored in zip - testTimestampsInZipEntries: Verifies zip entry timestamps correct - testMavenWarningScenario: Reproduces Maven warning about file timestamps - testMultipleFilesTimestampConsistency: Verifies consistent timestamps Test Results: - All 6 tests pass with fixes applied - Tests are Java 8 compatible using helper methods - Clear failure messages with timestamp diffs for debugging Fixes apache#387 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
0b0ea6e to
7b912ce
Compare
Addresses feedback from PR apache#388 review comments and adds comprehensive unit tests to validate timestamp preservation through cache operations. Changes to CacheUtils.java: 1. Implement glob filtering for directory entries - Track directories containing matching files during visitFile() - Only add directory entries in postVisitDirectory() if they contain matching files or no glob filter is active - Preserves glob pattern contract while maintaining timestamp preservation 2. Fix Files.exists() race condition (TOCTOU vulnerability) - Remove unnecessary Files.exists() checks before Files.createDirectories() - Leverage idempotent behavior of createDirectories() - Eliminates time-of-check-to-time-of-use race condition 3. Add error handling for timestamp operations - Wrap Files.setLastModifiedTime() in try-catch blocks - Best-effort timestamp setting with graceful degradation - Prevents failures on filesystems without timestamp support New Unit Tests (CacheUtilsTimestampTest.java): - testFileTimestampPreservation: Verifies file timestamps preserved - testDirectoryTimestampPreservation: Verifies directory timestamps preserved - testDirectoryEntriesStoredInZip: Verifies directories stored in zip - testTimestampsInZipEntries: Verifies zip entry timestamps correct - testMavenWarningScenario: Reproduces Maven warning about file timestamps - testMultipleFilesTimestampConsistency: Verifies consistent timestamps Test Results: - All 6 tests pass with fixes applied - Tests are Java 8 compatible using helper methods - Clear failure messages with timestamp diffs for debugging Fixes apache#387 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
7b912ce to
eb1e4da
Compare
Addresses feedback from PR apache#388 review comments and adds comprehensive unit tests to validate timestamp preservation through cache operations. Changes to CacheUtils.java: 1. Implement glob filtering for directory entries - Track directories containing matching files during visitFile() - Only add directory entries in postVisitDirectory() if they contain matching files or no glob filter is active - Preserves glob pattern contract while maintaining timestamp preservation 2. Fix Files.exists() race condition (TOCTOU vulnerability) - Remove unnecessary Files.exists() checks before Files.createDirectories() - Leverage idempotent behavior of createDirectories() - Eliminates time-of-check-to-time-of-use race condition 3. Add error handling for timestamp operations - Wrap Files.setLastModifiedTime() in try-catch blocks - Best-effort timestamp setting with graceful degradation - Prevents failures on filesystems without timestamp support New Unit Tests (CacheUtilsTimestampTest.java): - testFileTimestampPreservation: Verifies file timestamps preserved - testDirectoryTimestampPreservation: Verifies directory timestamps preserved - testDirectoryEntriesStoredInZip: Verifies directories stored in zip - testTimestampsInZipEntries: Verifies zip entry timestamps correct - testMavenWarningScenario: Reproduces Maven warning about file timestamps - testMultipleFilesTimestampConsistency: Verifies consistent timestamps Test Results: - All 6 tests pass with fixes applied - Tests are Java 8 compatible using helper methods - Clear failure messages with timestamp diffs for debugging Fixes apache#387 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
078171b to
6eee251
Compare
|
@olamy @AlexanderAshitkin Please see the updated commit. @AlexanderAshitkin Sorry for basing #388 on top of this PR. I have corrected the problem so they are now independent of each other. @olamy https://github.com/apache/maven-build-cache-extension/pull/387/files#diff-090be5e498346368594f96199eab418ac5486df7368b0825ad1db7f63ef5fff1R214 contains instructions for reproducing the Maven warning message. Please let me know if you are able to reproduce it on your end. Thanks. |
467c204 to
d9fc24f
Compare
|
Can you resolve conflicts and repush? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes timestamp preservation in the Maven Build Cache Extension to prevent Maven warnings about files being "more recent than the packaged artifact" when restoring outputs from cache.
Key Changes:
- Modified
CacheUtils.zip()to store directory entries with timestamps in ZIP archives - Updated
CacheUtils.unzip()to defer directory timestamp setting until after all files are extracted - Added configuration option
preserveTimestamps(default: true) for attached outputs
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/java/org/apache/maven/buildcache/CacheUtils.java |
Core fix: Added directory entry handling in zip/unzip with deferred timestamp restoration |
src/main/java/org/apache/maven/buildcache/xml/CacheConfig.java |
Added interface method for isPreserveTimestamps() configuration |
src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java |
Implemented isPreserveTimestamps() with default value handling |
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java |
Updated zip/unzip calls to use the new preserveTimestamps parameter |
src/main/mdo/build-cache-config.mdo |
Added configuration field for timestamp preservation with documentation |
src/test/java/org/apache/maven/buildcache/CacheUtilsTimestampTest.java |
Comprehensive test suite verifying timestamp preservation behavior |
src/test/java/org/apache/maven/buildcache/ModuleInfoCachingTest.java |
Test ensuring module-info.class files are properly cached |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test/java/org/apache/maven/buildcache/CacheUtilsTimestampTest.java
Outdated
Show resolved
Hide resolved
The build cache extension was not properly preserving file and directory timestamps when restoring attachedOutputs from cache. This caused Maven to warn about files being 'more recent than the packaged artifact' even after a successful build. Root Causes: 1. The zip() method did not store directory entries with timestamps 2. The unzip() method set directory timestamps immediately, but they were later modified by Files.copy() operations for files within Changes: - Modified CacheUtils.zip() to store directory entries with timestamps via preVisitDirectory() callback - Modified CacheUtils.unzip() to defer directory timestamp updates until after all files are extracted, preventing them from being overwritten - Added Map<Path, Long> to track directory timestamps during extraction This ensures that cached build outputs maintain their original timestamps, preventing spurious warnings and improving build cache consistency.
…tion Addresses feedback from PR apache#388 review comments and adds comprehensive testing and configuration capabilities for timestamp preservation. Changes: 1. Fix glob filtering for directory entries (PR apache#388 feedback) - Track directories containing matching files during visitFile() - Only add directory entries in postVisitDirectory() if they contain matching files or no glob filter is active - Preserves glob pattern contract while maintaining timestamp preservation 2. Fix Files.exists() race condition (PR apache#388 feedback) - Remove unnecessary Files.exists() checks before Files.createDirectories() - Leverage idempotent behavior of createDirectories() - Eliminates time-of-check-to-time-of-use (TOCTOU) vulnerability 3. Add error handling for timestamp operations (PR apache#388 feedback) - Wrap Files.setLastModifiedTime() in try-catch blocks - Best-effort timestamp setting with graceful degradation - Prevents failures on filesystems that don't support modification times 4. Add configuration property to control timestamp preservation - New preserveTimestamps field in AttachedOutputs configuration - Default value: true (timestamp preservation enabled by default) - Allows users to disable for performance if needed - Usage: <attachedOutputs><preserveTimestamps>false</preserveTimestamps></attachedOutputs> 5. Add comprehensive unit tests (CacheUtilsTimestampTest.java) - testFileTimestampPreservation: Verifies file timestamps preserved - testDirectoryTimestampPreservation: Verifies directory timestamps preserved - testDirectoryEntriesStoredInZip: Verifies directories stored in zip - testTimestampsInZipEntries: Verifies zip entry timestamps correct - testMavenWarningScenario: Reproduces Maven warning with manual repro steps - testMultipleFilesTimestampConsistency: Verifies consistent timestamps - testPreserveTimestampsFalse: Verifies configuration property works when disabled 6. Add test for module-info.class handling (ModuleInfoCachingTest.java) - Verifies module-info.class is preserved through zip/unzip operations All tests pass (7 tests in CacheUtilsTimestampTest.java).
|
Conflicts have been resolved and the branch has been rebased onto the latest master. The PR now includes both timestamp preservation (from this PR) and Unix permission preservation (from master) working together. |
d9fc24f to
2b4a88f
Compare
- Update exception handling comments to accurately reflect behavior (silently ignore failures rather than logging) - Remove redundant assertEquals in assertTimestampPreserved test method that could never fail
Problem
The Maven Build Cache Extension was not properly preserving file and directory timestamps when restoring
attachedOutputsfrom cache. This caused Maven to warn about files being 'more recent than the packaged artifact' even after runningmvnw verify.Root Causes
CacheUtils.zip()did not store directory entries with their timestamps - only files were added to the ZIPCacheUtils.unzip()set directory timestamps immediately during extraction, but these timestamps were subsequently overwritten whenFiles.copy()created files within those directoriesChanges
CacheUtils.zip()
preVisitDirectory()override to store directory entries in the ZIP with their original timestampsvisitFile()to explicitly set file entry timestamps fromBasicFileAttributesCacheUtils.unzip()
Map<Path, Long> directoryTimestampsto defer directory timestamp updatesHashMapandMapimports to support this functionalityTesting
Created comprehensive test that verifies:
Impact
This fix ensures that cached build outputs maintain their original timestamps, eliminating spurious Maven warnings and improving build cache consistency across multi-module projects.
Fixes timestamp-related warnings like:
Related Issues and PRs
This PR improves the ZIP archive handling in
CacheUtils:This fix specifically addresses timestamp preservation which was not covered by previous restoration improvements.