[Remote Cache] Don't delete tree artifacts on failure #6851
Conversation
Previously if a remote cache request failed, bazel would cleanup all directories that were marked as outputs for the action. These directories wouldn't end up being re-created by bazel, and could lead to failures if the actions didn't create the directories themselves. With this patch bazel deletes all output files, and the child directories of all output directories.
src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
Show resolved
Hide resolved
| FileSystemUtils.deleteTree(execRoot.getRelative(directory.getPath())); | ||
| // Only delete the directories below the output directories because the output | ||
| // directories will not be re-created | ||
| FileSystemUtils.deleteTreesBelow(execRoot.getRelative(directory.getPath())); |
There was a problem hiding this comment.
just had a thought, this method's name and documentation implies that it deletes only the subdirectories inside the given directory, but there may also be regular files rooted at this directory. @buchgr can you confirm that it deletes the files as well? I traced through the code, but wasn't sure.
There was a problem hiding this comment.
Would there ever be regular files that weren't part of the output? The output ones are deleted above, but otherwise I can stop using this helper and directly loop over the children of this directory
There was a problem hiding this comment.
Another option would be to leave the original line, and call createDirectoryAndParents afterwards
There was a problem hiding this comment.
I'll let @buchgr decide what's best.
There was a problem hiding this comment.
This line looks good to me. It deletes everything in a directory, including files symlinks and subdirectories. I agree that the naming could be improved.
|
@buchgr could this be prioritized? we're seeing errors that look similar to the original issue in https://buildkite.com/bazel/rules-apple-darwin/builds/237#049b31a0-3f26-4a84-abe0-971b3bccfe84 |
|
@keith Thanks a lot for the PR! I assume you test that this change fixes your issue? :-) |
|
Thanks! Yes it does! |
|
importing it. should be in master by tomorrow (need someone to lgtm internally). |
|
Thanks!
…--
Keith Smiley
On Dec 9, 2018, at 10:41, Jakob Buchgraber ***@***.***> wrote:
importing it. should be in master by tomorrow (need someone to lgtm internally).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Previously if a remote cache request failed, bazel would cleanup all
directories that were marked as outputs for the action. These
directories wouldn't end up being re-created by bazel, and could lead to
failures if the actions didn't create the directories themselves. With
this patch bazel deletes all output files, and the child directories of
all output directories.
Fixes #6260