Let RemoteOutputService ensure bazel-out/ is not a symlink

When doing a regular build without passing in --remote_download_*,
ExecutionTool will automatically clean up any traces of an OutputService
that replaces bazel-out/ with a symlink pointing to a remote file
system.

The --remote_download_* option is implemented by installing an
OutputService of its own, meaning that the regular logic in
ExecutionTool is skipped. The rationale being that an OutputService
essentially takes ownership of bazel-out/.

This change extends RemoteOutputService to be a more complete
implementation of OutputService that ensures that bazel-out/ is set up
properly. This improves interoperability with changes such as the gRPC
based Remote Output Service protocol (see bazelbuild#12823).

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

@@ -141,6 +141,7 @@ public final class RemoteModule extends BlazeModule {
   @Nullable private RemoteActionContextProvider actionContextProvider;
   @Nullable private RemoteActionInputFetcher actionInputFetcher;
   @Nullable private RemoteOptions remoteOptions;
+  @Nullable private CommandEnvironment env;
   @Nullable private RemoteOutputService remoteOutputService;
   @Nullable private TempPathGenerator tempPathGenerator;
   @Nullable private BlockWaitingModule blockWaitingModule;
@@ -313,6 +314,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
     }
 
     this.remoteOptions = remoteOptions;
+    this.env = env;
 
     AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class);
     DigestHashFunction hashFn = env.getRuntime().getFileSystem().getDigestFunction();
@@ -914,6 +916,7 @@ public void afterCommand() {
     actionContextProvider = null;
     actionInputFetcher = null;
     remoteOptions = null;
+    env = null;
     remoteOutputService = null;
     tempPathGenerator = null;
     rpcLogFile = null;
@@ -1067,7 +1070,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
   public OutputService getOutputService() {
     Preconditions.checkState(remoteOutputService == null, "remoteOutputService must be null");
     if (actionContextProvider.getRemoteCache() != null) {
-      remoteOutputService = new RemoteOutputService();
+      remoteOutputService = new RemoteOutputService(env);
     }
     return remoteOutputService;
   }

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

@@ -34,11 +34,17 @@
 import com.google.devtools.build.lib.actions.cache.OutputMetadataStore;
 import com.google.devtools.build.lib.buildtool.buildevent.ExecutionPhaseCompleteEvent;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.runtime.CommandEnvironment;
+import com.google.devtools.build.lib.server.FailureDetails.Execution;
+import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
+import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.util.AbruptExitException;
+import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.lib.vfs.BatchStat;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.lib.vfs.OutputService;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
@@ -51,11 +57,17 @@
 /** Output service implementation for the remote module */
 public class RemoteOutputService implements OutputService {
 
+  private final CommandEnvironment env;
+
   @Nullable private RemoteOutputChecker remoteOutputChecker;
   @Nullable private RemoteActionInputFetcher actionInputFetcher;
   @Nullable private LeaseService leaseService;
   @Nullable private Supplier<InputMetadataProvider> fileCacheSupplier;
 
+  public RemoteOutputService(CommandEnvironment env) {
+    this.env = checkNotNull(env);
+  }
+
   void setRemoteOutputChecker(RemoteOutputChecker remoteOutputChecker) {
     this.remoteOutputChecker = remoteOutputChecker;
   }
@@ -118,6 +130,24 @@ public String getFilesSystemName() {
   @Override
   public ModifiedFileSet startBuild(
       EventHandler eventHandler, UUID buildId, boolean finalizeActions) throws AbruptExitException {
+    // One of the responsibilities of OutputService.startBuild() is that
+    // it ensures the output path is valid. If the previous
+    // OutputService redirected the output path to a remote location, we
+    // must undo this.
+    Path outputPath = env.getDirectories().getOutputPath(env.getWorkspaceName());
+    if (outputPath.isSymbolicLink()) {
+      try {
+        outputPath.delete();
+      } catch (IOException e) {
+        throw new AbruptExitException(
+            DetailedExitCode.of(
+                FailureDetail.newBuilder()
+                    .setMessage(String.format("Couldn't remove output path symlink: %s", e.getMessage()))
+                    .setExecution(Execution.newBuilder().setCode(Code.LOCAL_OUTPUT_DIRECTORY_SYMLINK_FAILURE))
+                    .build()),
+            e);
+      }
+    }
     return ModifiedFileSet.EVERYTHING_MODIFIED;
   }
 

src/test/shell/bazel/remote/build_without_the_bytes_test.sh

@@ -2049,4 +2049,35 @@ EOF
   expect_log "Hello World"
 }
 
+function test_output_path_is_symlink() {
+  cat > BUILD << 'EOF'
+genrule(
+  name = "foo",
+  outs = ["bar"],
+  cmd = "touch $@",
+)
+EOF
+  bazel build \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_download_minimal \
+    //:foo >& $TEST_log || fail "Failed to build //:foo"
+
+  # --remote_download_minimal and --remote_download_toplevel install an
+  # OutputService. One of the responsibilities of an OutputService is
+  # that it ensures a valid output path is present.
+  #
+  # Simulate the case where another OutputService replaced the output
+  # path with a symbolic link. If Bazel is rerun with
+  # --remote_download_minimal, it should remove the symbolic link, so
+  # that builds can take place on the local file system.
+  output_path=$(bazel info output_path)
+  rm -rf $output_path
+  ln -s /nonexistent $output_path
+
+  bazel build \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_download_minimal \
+    //:foo >& $TEST_log || fail "Failed to build //:foo"
+}
+
 run_suite "Build without the Bytes tests"