Fix disk cache failures on concurrent read-write access on Windows #28417
Conversation
453bb2c to
086cf9c
Compare
src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java
Show resolved
Hide resolved
b639ce2 to
89bac4a
Compare
|
@bazel-io fork 8.6.0 |
|
@bazel-io fork 9.0.1 |
|
@bazel-io fork 9.1.0 |
There was a problem hiding this comment.
Code Review
This pull request applies a fix for concurrent read-write access on Windows to the disk cache, mirroring a previous fix in the download cache. The changes correctly handle FileAccessException on Windows during file renames in concurrent scenarios. The new test case is well-designed to validate this fix. I've identified a few areas for improvement related to resource cleanup and error handling to make the implementation more robust.
| if (OS.getCurrent() != OS.WINDOWS || !cacheValue.exists()) { | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
In DiskCacheClient, you've added logic to delete the temporary file in this situation, which is great for preventing resource leaks. A similar cleanup for tmpName seems to be missing here. When this FileAccessException is caught and handled as a benign race, the temporary file tmpName should be deleted to avoid leaving it behind in the cache directory.
| if (OS.getCurrent() != OS.WINDOWS || !target.exists()) { | ||
| throw e; | ||
| } | ||
| src.delete(); |
There was a problem hiding this comment.
If src.delete() throws an IOException, the entire operation will fail. Since we've determined this is a benign race condition where another thread has already successfully cached the file, this operation should not fail. The cleanup of the temporary file should be on a best-effort basis.
try {
src.delete();
} catch (IOException ignored) {
// Best-effort cleanup of the temporary file.
}| if (OS.getCurrent() != OS.WINDOWS || !path.exists()) { | ||
| throw e; | ||
| } | ||
| temp.delete(); |
There was a problem hiding this comment.
If temp.delete() throws an IOException, the entire operation will fail. Since we've determined this is a benign race condition where another thread has already successfully cached the file, this operation should not fail. The cleanup of the temporary file should be on a best-effort basis.
try {
temp.delete();
} catch (IOException ignored) {
// Best-effort cleanup of the temporary file.
}
src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java
Outdated
Show resolved
Hide resolved
This applies the fix made to the download cache in 753dc97 to the disk cache.
89bac4a to
e0a92ad
Compare
This applies the fix made to the download cache in 753dc97 to the disk cache.
Work towards #28408