diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 00001e90186828..eb55f4f40a2a6d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -849,6 +849,12 @@ private void createSymlinks(Iterable symlinks) throws IOExcepti "Failed creating directory and parents for %s", symlink.path()) .createDirectoryAndParents(); + // If a directory output is being materialized as a symlink, we must first delete the + // preexisting empty directory. + if (symlink.path().exists(Symlinks.NOFOLLOW) + && symlink.path().isDirectory(Symlinks.NOFOLLOW)) { + symlink.path().delete(); + } symlink.path().createSymbolicLink(symlink.target()); } } @@ -1166,12 +1172,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re Map realToTmpPath = new HashMap<>(); if (downloadOutputs) { - // Create output directories first. - // This ensures that the directories are present even if downloading fails. - // See https://github.com/bazelbuild/bazel/issues/6260. - for (Entry entry : metadata.directories()) { - entry.getKey().createDirectoryAndParents(); - } downloadsBuilder.addAll( buildFilesToDownload(context, progressStatusListener, metadata, realToTmpPath)); } else { diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index ad8899c76b7066..b2845ab99fa867 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -303,6 +303,7 @@ public void downloadOutputs_outputFiles_executableBitIgnored() throws Exception FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -331,6 +332,7 @@ public void downloadOutputs_siblingLayoutAndRelativeToInputRoot_works() throws E FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -396,6 +398,7 @@ public void downloadOutputs_emptyOutputDirectories_works() throws Exception { FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -442,6 +445,7 @@ public void downloadOutputs_nestedOutputDirectories_works() throws Exception { FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -479,6 +483,7 @@ public void downloadOutputs_outputDirectoriesWithNestedFile_works() throws Excep FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -521,13 +526,14 @@ public void downloadOutputs_outputDirectoriesWithSameHash_works() throws Excepti .build(); Digest treeDigest = cache.addContents(remoteActionExecutionContext, tree.toByteArray()); ActionResult.Builder builder = ActionResult.newBuilder(); - builder.addOutputDirectoriesBuilder().setPath("outputs/a/").setTreeDigest(treeDigest); + builder.addOutputDirectoriesBuilder().setPath("outputs/a").setTreeDigest(treeDigest); RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build())); Spawn spawn = newSpawnFromResult(result); FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act service.downloadOutputs(action, result); @@ -570,6 +576,7 @@ public void downloadOutputs_relativeDirectorySymlink_success() throws Exception FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // Doesn't check for dangling links, hence download succeeds. service.downloadOutputs(action, result); @@ -592,6 +599,7 @@ public void downloadOutputs_relativeOutputSymlinks_success() throws Exception { FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // Doesn't check for dangling links, hence download succeeds. service.downloadOutputs(action, result); @@ -618,6 +626,7 @@ public void downloadOutputs_outputSymlinksCompatibility_success() throws Excepti FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // Doesn't check for dangling links, hence download succeeds. service.downloadOutputs(action, result); @@ -647,6 +656,7 @@ public void downloadOutputs_relativeSymlinkInDirectory_success() throws Exceptio remoteOptions.incompatibleRemoteDisallowSymlinkInTreeArtifact = false; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // Doesn't check for dangling links, hence download succeeds. service.downloadOutputs(action, result); @@ -667,6 +677,7 @@ public void downloadOutputs_absoluteFileSymlink_success() throws Exception { FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); service.downloadOutputs(action, result); @@ -686,6 +697,7 @@ public void downloadOutputs_absoluteDirectorySymlink_success() throws Exception FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); service.downloadOutputs(action, result); @@ -711,6 +723,7 @@ public void downloadOutputs_absoluteSymlinkInDirectory_error() throws Exception FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); IOException expected = assertThrows(IOException.class, () -> service.downloadOutputs(action, result)); @@ -743,37 +756,44 @@ public void downloadOutputs_symlinkCollision_error() throws Exception { assertThat(expected).hasMessageThat().contains("foo1"); assertThat(expected).hasMessageThat().contains("foo2"); } - + @Test public void downloadOutputs_onFailure_maintainDirectories() throws Exception { - // Test that output directories are not deleted on download failure. See - // https://github.com/bazelbuild/bazel/issues/6260. - Tree tree = Tree.newBuilder().setRoot(Directory.getDefaultInstance()).build(); + // Test that output directories created prior to spawn execution are not deleted on failure. + Digest treeFileDigest = + cache.addException("outputs/outputdir/outputfile", new IOException("download failed")); + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addFiles( + FileNode.newBuilder() + .setName("outputfile") + .setDigest(treeFileDigest) + .setIsExecutable(true))) + .build(); Digest treeDigest = cache.addContents(remoteActionExecutionContext, tree.toByteArray()); - Digest outputFileDigest = - cache.addException("outputdir/outputfile", new IOException("download failed")); - Digest otherFileDigest = cache.addContents(remoteActionExecutionContext, "otherfile"); + Digest otherFileDigest = + cache.addException("outputs/otherdir/otherfile", new IOException("download failed")); ActionResult.Builder builder = ActionResult.newBuilder(); builder.addOutputDirectoriesBuilder().setPath("outputs/outputdir").setTreeDigest(treeDigest); builder.addOutputFiles( - OutputFile.newBuilder() - .setPath("outputs/outputdir/outputfile") - .setDigest(outputFileDigest)); - builder.addOutputFiles( - OutputFile.newBuilder().setPath("outputs/otherfile").setDigest(otherFileDigest)); + OutputFile.newBuilder().setPath("outputs/otherdir/otherfile").setDigest(otherFileDigest)); RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build())); Spawn spawn = newSpawnFromResult(result); FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); assertThrows(BulkTransferException.class, () -> service.downloadOutputs(action, result)); - assertThat(cache.getNumFailedDownloads()).isEqualTo(1); + assertThat(cache.getNumFailedDownloads()).isEqualTo(2); assertThat(execRoot.getRelative("outputs/outputdir").exists()).isTrue(); assertThat(execRoot.getRelative("outputs/outputdir/outputfile").exists()).isFalse(); - assertThat(execRoot.getRelative("outputs/otherfile").exists()).isFalse(); + assertThat(execRoot.getRelative("outputs/otherdir").exists()).isTrue(); + assertThat(execRoot.getRelative("outputs/otherdir/otherfile").exists()).isFalse(); assertThat(context.isLockOutputFilesCalled()).isFalse(); } @@ -798,6 +818,7 @@ public void downloadOutputs_onError_waitForRemainingDownloadsToComplete() throws FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); BulkTransferException downloadException = assertThrows(BulkTransferException.class, () -> service.downloadOutputs(action, result)); @@ -829,6 +850,7 @@ public void downloadOutputs_withMultipleErrors_addsThemAsSuppressed() throws Exc FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); BulkTransferException e = assertThrows(BulkTransferException.class, () -> service.downloadOutputs(action, result)); @@ -859,6 +881,7 @@ public void downloadOutputs_withDuplicateIOErrors_doesNotSuppress() throws Excep FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); BulkTransferException downloadException = assertThrows(BulkTransferException.class, () -> service.downloadOutputs(action, result)); @@ -889,6 +912,7 @@ public void downloadOutputs_withDuplicateInterruptions_doesNotSuppress() throws FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); InterruptedException e = assertThrows(InterruptedException.class, () -> service.downloadOutputs(action, result)); @@ -919,6 +943,7 @@ public void downloadOutputs_withStdoutStderrOnSuccess_writable() throws Exceptio FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn, spyOutErr); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); service.downloadOutputs(action, result); @@ -959,6 +984,7 @@ public void downloadOutputs_withStdoutStderrOnFailure_writableAndEmpty() throws FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn, spyOutErr); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); assertThrows(BulkTransferException.class, () -> service.downloadOutputs(action, result)); @@ -991,6 +1017,7 @@ public void downloadOutputs_outputNameClashesWithTempName_success() throws Excep FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); service.downloadOutputs(action, result); @@ -1019,6 +1046,7 @@ public void downloadOutputs_outputFilesWithoutTopLevel_inject() throws Exception remoteOptions.remoteOutputsMode = RemoteOutputsMode.TOPLEVEL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1059,6 +1087,7 @@ public void downloadOutputs_outputFilesWithMinimal_injectingMetadata() throws Ex remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1121,6 +1150,7 @@ public void downloadOutputs_outputDirectoriesWithMinimal_injectingMetadata() thr remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1139,8 +1169,7 @@ public void downloadOutputs_outputDirectoriesWithMinimal_injectingMetadata() thr eq(toBinaryDigest(d2)), eq(d2.getSizeBytes()), anyLong()); - Path outputBase = checkNotNull(artifactRoot.getRoot().asPath()); - assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); + assertThat(execRoot.getRelative("outputs/dir").readdir(Symlinks.NOFOLLOW)).isEmpty(); assertThat(context.isLockOutputFilesCalled()).isTrue(); } @@ -1185,6 +1214,7 @@ public void downloadOutputs_outputDirectoriesWithMinimalOnFailure_failProperly() remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act BulkTransferException e = @@ -1216,6 +1246,7 @@ public void downloadOutputs_nonInlinedStdoutAndStderrWithMinimal_works() throws remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1255,6 +1286,7 @@ public void downloadOutputs_inlinedStdoutAndStderrWithMinimal_works() throws Exc remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1298,6 +1330,7 @@ public void downloadOutputs_inMemoryOutputWithMinimal_downloadIt() throws Except remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1358,6 +1391,7 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw remoteOptions.remoteOutputsMode = RemoteOutputsMode.MINIMAL; RemoteExecutionService service = newRemoteExecutionService(remoteOptions); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); // act InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); @@ -1392,6 +1426,7 @@ public void downloadOutputs_missingMandatoryOutputs_reportError() throws Excepti FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); RemoteExecutionService service = newRemoteExecutionService(); RemoteAction action = service.buildRemoteAction(spawn, context); + createOutputDirectories(spawn); IOException error = assertThrows(IOException.class, () -> service.downloadOutputs(action, result)); @@ -2150,4 +2185,14 @@ private RemoteExecutionService newRemoteExecutionService(RemoteOptions remoteOpt null, DUMMY_REMOTE_OUTPUT_CHECKER); } + + private void createOutputDirectories(Spawn spawn) throws IOException { + for (ActionInput input : spawn.getOutputFiles()) { + Path dir = execRoot.getRelative(input.getExecPath()); + if (!input.isDirectory()) { + dir = dir.getParentDirectory(); + } + dir.createDirectoryAndParents(); + } + } }