RemoteExecutionService: support output_symlinks in ActionResult (#18441)

Since Remote API v2.1, both
  - output_file_symlinks
  - output_directory_symlinks
were marked as deprecated in favor of output_symlinks.

However, Bazel downloadOutputs has only support the 2 deprecated fields
and not output_symlinks.

Add support for output_symlinks.

Closes #18198.

PiperOrigin-RevId: 527511382
Change-Id: I350ab77a5d30ab06dab4118cf76f0b0e7650a739

Co-authored-by: Son Luong Ngoc <[email protected]>

Author: iancha1992

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

@@ -210,7 +210,7 @@ public RemoteExecutionService(
     this.tempPathGenerator = tempPathGenerator;
     this.captureCorruptedOutputsDir = captureCorruptedOutputsDir;
 
-    this.scheduler = Schedulers.from(executor, /*interruptibleWorker=*/ true);
+    this.scheduler = Schedulers.from(executor, /* interruptibleWorker= */ true);
   }
 
   static Command buildCommand(
@@ -639,6 +639,10 @@ public List<OutputSymlink> getOutputDirectorySymlinks() {
       return actionResult.getOutputDirectorySymlinksList();
     }
 
+    public List<OutputSymlink> getOutputSymlinks() {
+      return actionResult.getOutputSymlinksList();
+    }
+
     /**
      * Returns the freeform informational message with details on the execution of the action that
      * may be displayed to the user upon failure or when requested explicitly.
@@ -1005,7 +1009,7 @@ ActionResultMetadata parseActionResultMetadata(
               directExecutor()));
     }
 
-    waitForBulkTransfer(dirMetadataDownloads.values(), /* cancelRemainingOnInterrupt=*/ true);
+    waitForBulkTransfer(dirMetadataDownloads.values(), /* cancelRemainingOnInterrupt= */ true);
 
     ImmutableMap.Builder<Path, DirectoryMetadata> directories = ImmutableMap.builder();
     for (Map.Entry<Path, ListenableFuture<Tree>> metadataDownload :
@@ -1029,18 +1033,33 @@ ActionResultMetadata parseActionResultMetadata(
           new FileMetadata(localPath, outputFile.getDigest(), outputFile.getIsExecutable()));
     }
 
-    ImmutableMap.Builder<Path, SymlinkMetadata> symlinks = ImmutableMap.builder();
-    Iterable<OutputSymlink> outputSymlinks =
-        Iterables.concat(result.getOutputFileSymlinks(), result.getOutputDirectorySymlinks());
-    for (OutputSymlink symlink : outputSymlinks) {
-      Path localPath =
+    var symlinkMap = new HashMap<Path, SymlinkMetadata>();
+    var outputSymlinks =
+        Iterables.concat(
+            result.getOutputFileSymlinks(),
+            result.getOutputDirectorySymlinks(),
+            result.getOutputSymlinks());
+    for (var symlink : outputSymlinks) {
+      var localPath =
           remotePathResolver.outputPathToLocalPath(encodeBytestringUtf8(symlink.getPath()));
-      symlinks.put(
-          localPath, new SymlinkMetadata(localPath, PathFragment.create(symlink.getTarget())));
+      var target = PathFragment.create(symlink.getTarget());
+      var existingMetadata = symlinkMap.get(localPath);
+      if (existingMetadata != null) {
+        if (!target.equals(existingMetadata.target())) {
+          throw new IOException(
+              String.format(
+                  "Symlink path collision: '%s' is mapped to both '%s' and '%s'. Action Result"
+                      + " should not contain multiple targets for the same symlink.",
+                  localPath, existingMetadata.target(), target));
+        }
+        continue;
+      }
+
+      symlinkMap.put(localPath, new SymlinkMetadata(localPath, target));
     }
 
     return new ActionResultMetadata(
-        files.buildOrThrow(), symlinks.buildOrThrow(), directories.buildOrThrow());
+        files.buildOrThrow(), ImmutableMap.copyOf(symlinkMap), directories.buildOrThrow());
   }
 
   /**
@@ -1156,7 +1175,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
           ListenableFuture<byte[]> inMemoryOutputDownload =
               remoteCache.downloadBlob(context, inMemoryOutputDigest);
           waitForBulkTransfer(
-              ImmutableList.of(inMemoryOutputDownload), /* cancelRemainingOnInterrupt=*/ true);
+              ImmutableList.of(inMemoryOutputDownload), /* cancelRemainingOnInterrupt= */ true);
           byte[] data = getFromFuture(inMemoryOutputDownload);
           return new InMemoryOutput(inMemoryOutput, ByteString.copyFrom(data));
         }

src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java

@@ -575,6 +575,54 @@ public void downloadOutputs_relativeDirectorySymlink_success() throws Exception
     assertThat(context.isLockOutputFilesCalled()).isTrue();
   }
 
+  @Test
+  public void downloadOutputs_relativeOutputSymlinks_success() throws Exception {
+    // Test that download outputs works when the action result only contains output_symlinks
+    // and not output_file_symlinks or output_directory_symlinks, which are deprecated.
+    ActionResult.Builder builder = ActionResult.newBuilder();
+    builder.addOutputSymlinksBuilder().setPath("outputs/a/b/link").setTarget("../../foo");
+    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);
+
+    // Doesn't check for dangling links, hence download succeeds.
+    service.downloadOutputs(action, result);
+
+    Path path = execRoot.getRelative("outputs/a/b/link");
+    assertThat(path.isSymbolicLink()).isTrue();
+    assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("../../foo"));
+    assertThat(context.isLockOutputFilesCalled()).isTrue();
+  }
+
+  @Test
+  public void downloadOutputs_outputSymlinksCompatibility_success() throws Exception {
+    // Test that download outputs works when the action result contains both output_symlinks
+    // and output_file_symlinks (or output_directory_symlinks).
+    //
+    // Remote Execution Server may set both fields to ensure backward compatibility with
+    // clients that don't support output_symlinks.
+    ActionResult.Builder builder = ActionResult.newBuilder();
+    builder.addOutputFileSymlinksBuilder().setPath("outputs/a/b/link").setTarget("foo");
+    builder.addOutputSymlinksBuilder().setPath("outputs/a/b/link").setTarget("foo");
+    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);
+
+    // Doesn't check for dangling links, hence download succeeds.
+    service.downloadOutputs(action, result);
+
+    Path path = execRoot.getRelative("outputs/a/b/link");
+    assertThat(path.isSymbolicLink()).isTrue();
+    assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("foo"));
+    assertThat(context.isLockOutputFilesCalled()).isTrue();
+  }
+
   @Test
   public void downloadOutputs_relativeSymlinkInDirectory_success() throws Exception {
     Tree tree =
@@ -666,6 +714,28 @@ public void downloadOutputs_absoluteSymlinkInDirectory_error() throws Exception
     assertThat(context.isLockOutputFilesCalled()).isTrue();
   }
 
+  @Test
+  public void downloadOutputs_symlinkCollision_error() throws Exception {
+    ActionResult.Builder builder = ActionResult.newBuilder();
+    builder.addOutputDirectorySymlinksBuilder().setPath("outputs/foo").setTarget("foo1");
+    builder.addOutputSymlinksBuilder().setPath("outputs/foo").setTarget("foo2");
+    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);
+
+    IOException expected =
+        assertThrows(IOException.class, () -> service.downloadOutputs(action, result));
+
+    assertThat(expected.getSuppressed()).isEmpty();
+    assertThat(expected).hasMessageThat().contains("Symlink path collision");
+    assertThat(expected).hasMessageThat().contains("outputs/foo");
+    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
@@ -1948,6 +2018,12 @@ private Spawn newSpawnFromResult(
       outputs.add(output);
     }
 
+    for (OutputSymlink symlink : result.getOutputSymlinks()) {
+      Path path = remotePathResolver.outputPathToLocalPath(symlink.getPath());
+      Artifact output = ActionsTestUtil.createArtifact(artifactRoot, path);
+      outputs.add(output);
+    }
+
     return newSpawn(executionInfo, outputs.build());
   }