Fix downloading remote execution output files inside output dirs. (#1…

…5444) Adds a check to prevent creating multiple download futures for output files that are children of output directories. Fixes #15328 Closes #15329. PiperOrigin-RevId: 444542026 Co-authored-by: John Millikin <john@john-millikin.com>
bazelbuild · May 10, 2022 · 652b48e · 652b48e
1 parent a430042
commit 652b48e
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 2 deletions.
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
@@ -1048,13 +1048,21 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
             hasFilesToDownload(action.spawn.getOutputFiles(), filesToDownload));
 
     if (downloadOutputs) {
+      HashSet<PathFragment> queuedFilePaths = new HashSet<>();
+
       for (FileMetadata file : metadata.files()) {
-        downloadsBuilder.add(downloadFile(action, file));
+        PathFragment filePath = file.path().asFragment();
+        if (queuedFilePaths.add(filePath)) {
+          downloadsBuilder.add(downloadFile(action, file));
+        }
       }
 
       for (Map.Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
         for (FileMetadata file : entry.getValue().files()) {
-          downloadsBuilder.add(downloadFile(action, file));
+          PathFragment filePath = file.path().asFragment();
+          if (queuedFilePaths.add(filePath)) {
+            downloadsBuilder.add(downloadFile(action, file));
+          }
         }
       }
     } 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
@@ -360,6 +360,41 @@ public void downloadOutputs_nestedOutputDirectories_works() throws Exception {
     assertThat(context.isLockOutputFilesCalled()).isTrue();
   }
 
+  @Test
+  public void downloadOutputs_outputDirectoriesWithNestedFile_works() throws Exception {
+    // Test that downloading an output directory containing a named output file works.
+
+    // arrange
+    Digest fooDigest = cache.addContents(remoteActionExecutionContext, "foo-contents");
+    Digest barDigest = cache.addContents(remoteActionExecutionContext, "bar-ontents");
+    Tree subdirTreeMessage =
+        Tree.newBuilder()
+            .setRoot(
+                Directory.newBuilder()
+                    .addFiles(FileNode.newBuilder().setName("foo").setDigest(fooDigest))
+                    .addFiles(FileNode.newBuilder().setName("bar").setDigest(barDigest)))
+            .build();
+    Digest subdirTreeDigest =
+        cache.addContents(remoteActionExecutionContext, subdirTreeMessage.toByteArray());
+    ActionResult.Builder builder = ActionResult.newBuilder();
+    builder.addOutputFilesBuilder().setPath("outputs/subdir/foo").setDigest(fooDigest);
+    builder.addOutputDirectoriesBuilder().setPath("outputs/subdir").setTreeDigest(subdirTreeDigest);
+    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);
+
+    // act
+    service.downloadOutputs(action, result);
+
+    // assert
+    assertThat(digestUtil.compute(execRoot.getRelative("outputs/subdir/foo"))).isEqualTo(fooDigest);
+    assertThat(digestUtil.compute(execRoot.getRelative("outputs/subdir/bar"))).isEqualTo(barDigest);
+    assertThat(context.isLockOutputFilesCalled()).isTrue();
+  }
+
   @Test
   public void downloadOutputs_outputDirectoriesWithSameHash_works() throws Exception {
     // Test that downloading an output directory works when two Directory