Fix disk cache failures on concurrent read-write access on Windows#28417
Fix disk cache failures on concurrent read-write access on Windows#28417fmeum wants to merge 3 commits intobazelbuild:masterfrom
Conversation
fmeum
force-pushed
the
28408-concurrency-fix
branch
from
453bb2c to
086cf9c
Compare
src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java
Show resolved
Hide resolved
fmeum
force-pushed
the
28408-concurrency-fix
branch
from
b639ce2 to
89bac4a
Compare
|
@bazel-io fork 8.6.0 |
github-actions
bot
added
team-ExternalDeps
|
@bazel-io fork 9.0.1 |
|
@bazel-io fork 9.1.0 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
fmeum
force-pushed
the
28408-concurrency-fix
branch
from
89bac4a to
e0a92ad
Compare
coeuvre
added
awaiting-PR-merge
github-actions
bot
removed
the
awaiting-PR-merge
…azelbuild#28417) This applies the fix made to the download cache in 753dc97 to the disk cache. Work towards bazelbuild#28408 Closes bazelbuild#28417. PiperOrigin-RevId: 864861790 Change-Id: I6850fc27f3336f44f8d366daac63e4822cb94f73
…azelbuild#28417) This applies the fix made to the download cache in 753dc97 to the disk cache. Work towards bazelbuild#28408 Closes bazelbuild#28417. PiperOrigin-RevId: 864861790 Change-Id: I6850fc27f3336f44f8d366daac63e4822cb94f73
…azelbuild#28417) This applies the fix made to the download cache in 753dc97 to the disk cache. Work towards bazelbuild#28408 Closes bazelbuild#28417. PiperOrigin-RevId: 864861790 Change-Id: I6850fc27f3336f44f8d366daac63e4822cb94f73
…ndows (h… (#28529) …ttps://github.com//pull/28417) This applies the fix made to the download cache in 753dc97 to the disk cache. Work towards #28408 Closes #28417. PiperOrigin-RevId: 864861790 Change-Id: I6850fc27f3336f44f8d366daac63e4822cb94f73 Commit 73c8da8 --------- Co-authored-by: Fabian Meumertzheim <[email protected]>
…ndows (h… (#28552) …ttps://github.com//pull/28417) This applies the fix made to the download cache in 753dc97 to the disk cache. Work towards #28408 Closes #28417. PiperOrigin-RevId: 864861790 Change-Id: I6850fc27f3336f44f8d366daac63e4822cb94f73 Commit 73c8da8 --------- Co-authored-by: Fabian Meumertzheim <[email protected]> Co-authored-by: Tiago Quelhas <[email protected]>
…ndows (h… (#28555) …ttps://github.com//pull/28417) This applies the fix made to the download cache in 753dc97 to the disk cache. Work towards #28408 Closes #28417. PiperOrigin-RevId: 864861790 Change-Id: I6850fc27f3336f44f8d366daac63e4822cb94f73 Commit 73c8da8 --------- Co-authored-by: Fabian Meumertzheim <[email protected]>
2 participants
This applies the fix made to the download cache in 753dc97 to the disk cache.
Work towards #28408