From 652b48e567fcb30768dfc2eddee5f04bf6b5d65b Mon Sep 17 00:00:00 2001
From: Chenchu Kolli <ckolli@google.com>
Date: Mon, 9 May 2022 19:16:59 -0500
Subject: [PATCH] Fix downloading remote execution output files inside output
 dirs. (#15444)

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>
---
 .../lib/remote/RemoteExecutionService.java    | 12 +++++--
 .../remote/RemoteExecutionServiceTest.java    | 35 +++++++++++++++++++
 2 files changed, 45 insertions(+), 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
index e700715db90244..063ff493715393 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
@@ -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
index 18b5a688106763..dad364ca44b1be 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
@@ -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