Skip to content

Commit

Permalink
[Remote Cache] Don't delete tree artifacts on failure
Browse files Browse the repository at this point in the history
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

Closes #6851.

PiperOrigin-RevId: 224781649
  • Loading branch information
keith authored and Copybara-Service committed Dec 10, 2018
1 parent d6eb5de commit 4392ba4
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,9 @@ public void onFailure(Throwable t) {
execRoot.getRelative(file.getPath()).delete();
}
for (OutputDirectory directory : result.getOutputDirectoriesList()) {
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()));
}
if (outErr != null) {
outErr.getOutputPath().delete();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,31 @@ public void downloadAbsoluteSymlinkInDirectoryError() throws Exception {
}
}

@Test
public void downloadFailureMaintainsDirectories() throws Exception {
DefaultRemoteActionCache cache = newTestCache();
Tree tree = Tree.newBuilder().setRoot(Directory.newBuilder()).build();
Digest treeDigest = cache.addContents(tree.toByteArray());
Digest outputFileDigest =
cache.addException("outputdir/outputfile", new IOException("download failed"));
Digest otherFileDigest = cache.addContents("otherfile");

ActionResult.Builder result = ActionResult.newBuilder();
result.addOutputDirectoriesBuilder().setPath("outputdir").setTreeDigest(treeDigest);
result.addOutputFiles(
OutputFile.newBuilder().setPath("outputdir/outputfile").setDigest(outputFileDigest));
result.addOutputFiles(OutputFile.newBuilder().setPath("otherfile").setDigest(otherFileDigest));
try {
cache.download(result.build(), execRoot, null);
fail("Expected exception");
} catch (IOException expected) {
assertThat(cache.getNumFailedDownloads()).isEqualTo(1);
assertThat(execRoot.getRelative("outputdir").exists()).isTrue();
assertThat(execRoot.getRelative("outputdir/outputfile").exists()).isFalse();
assertThat(execRoot.getRelative("otherfile").exists()).isFalse();
}
}

@Test
public void onErrorWaitForRemainingDownloadsToComplete() throws Exception {
// If one or more downloads of output files / directories fail then the code should
Expand Down

0 comments on commit 4392ba4

Please sign in to comment.