Temporarily set parent directory of the input to writable if it is not.

coeuvre · copybara-github · commit ec7be346adc0 · 2022-10-25T02:29:05.000-07:00
PiperOrigin-RevId: 483615392
Change-Id: Ic8b301ef6fd004a88f9a563e04ba961f6dfb5708
diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
@@ -70,6 +70,20 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet
   protected final Path execRoot;
   protected final ImmutableList<Pattern> patternsToDownload;
 
+  private static class Context {
+    private final Set<Path> nonWritableDirs = Sets.newConcurrentHashSet();
+
+    public void addNonWritableDir(Path dir) {
+      nonWritableDirs.add(dir);
+    }
+
+    public void finalizeContext() throws IOException {
+      for (Path path : nonWritableDirs) {
+        path.setWritable(false);
+      }
+    }
+  }
+
   /** Priority for the staging task. */
   protected enum Priority {
     /**
@@ -176,27 +190,37 @@ protected ListenableFuture<Void> prefetchFiles(
       files.add(input);
     }
 
+    Context context = new Context();
+
     Flowable<TransferResult> treeDownloads =
         Flowable.fromIterable(trees.entrySet())
             .flatMapSingle(
                 entry ->
                     toTransferResult(
                         prefetchInputTreeOrSymlink(
-                            metadataProvider, entry.getKey(), entry.getValue(), priority)));
+                            context,
+                            metadataProvider,
+                            entry.getKey(),
+                            entry.getValue(),
+                            priority)));
 
     Flowable<TransferResult> fileDownloads =
         Flowable.fromIterable(files)
             .flatMapSingle(
                 input ->
                     toTransferResult(
-                        prefetchInputFileOrSymlink(metadataProvider, input, priority)));
+                        prefetchInputFileOrSymlink(context, metadataProvider, input, priority)));
 
     Flowable<TransferResult> transfers = Flowable.merge(treeDownloads, fileDownloads);
-    Completable prefetch = mergeBulkTransfer(transfers).onErrorResumeNext(this::onErrorResumeNext);
+    Completable prefetch =
+        Completable.using(
+                () -> context, ctx -> mergeBulkTransfer(transfers), Context::finalizeContext)
+            .onErrorResumeNext(this::onErrorResumeNext);
     return toListenableFuture(prefetch);
   }
 
   private Completable prefetchInputTreeOrSymlink(
+      Context context,
       MetadataProvider provider,
       SpecialArtifact tree,
       List<TreeFileArtifact> treeFiles,
@@ -216,7 +240,7 @@ private Completable prefetchInputTreeOrSymlink(
     PathFragment prefetchExecPath = treeMetadata.getMaterializationExecPath().orElse(execPath);
 
     Completable prefetch =
-        prefetchInputTree(provider, prefetchExecPath, treeFiles, treeMetadata, priority);
+        prefetchInputTree(context, provider, prefetchExecPath, treeFiles, treeMetadata, priority);
 
     // If prefetching to a different path, plant a symlink into it.
     if (!prefetchExecPath.equals(execPath)) {
@@ -249,6 +273,7 @@ private boolean shouldDownloadAnyTreeFiles(
   }
 
   private Completable prefetchInputTree(
+      Context context,
       MetadataProvider provider,
       PathFragment execPath,
       List<TreeFileArtifact> treeFiles,
@@ -303,7 +328,7 @@ private Completable prefetchInputTree(
                       }
                     }
                     checkState(dir.equals(path));
-                    finalizeDownload(tempPath, path);
+                    finalizeDownload(context, tempPath, path);
                   }
 
                   for (Path dir : dirs) {
@@ -337,7 +362,8 @@ private Completable prefetchInputTree(
   }
 
   private Completable prefetchInputFileOrSymlink(
-      MetadataProvider metadataProvider, ActionInput input, Priority priority) throws IOException {
+      Context context, MetadataProvider metadataProvider, ActionInput input, Priority priority)
+      throws IOException {
     if (input instanceof VirtualActionInput) {
       prefetchVirtualActionInput((VirtualActionInput) input);
       return Completable.complete();
@@ -353,7 +379,7 @@ private Completable prefetchInputFileOrSymlink(
     PathFragment prefetchExecPath = metadata.getMaterializationExecPath().orElse(execPath);
 
     Completable prefetch =
-        downloadFileNoCheckRx(execRoot.getRelative(prefetchExecPath), metadata, priority);
+        downloadFileNoCheckRx(context, execRoot.getRelative(prefetchExecPath), metadata, priority);
 
     // If prefetching to a different path, plant a symlink into it.
     if (!prefetchExecPath.equals(execPath)) {
@@ -371,15 +397,16 @@ private Completable prefetchInputFileOrSymlink(
    * <p>The file will be written into a temporary file and moved to the final destination after the
    * download finished.
    */
-  private Completable downloadFileRx(Path path, FileArtifactValue metadata, Priority priority) {
+  private Completable downloadFileRx(
+      Context context, Path path, FileArtifactValue metadata, Priority priority) {
     if (!canDownloadFile(path, metadata)) {
       return Completable.complete();
     }
-    return downloadFileNoCheckRx(path, metadata, priority);
+    return downloadFileNoCheckRx(context, path, metadata, priority);
   }
 
   private Completable downloadFileNoCheckRx(
-      Path path, FileArtifactValue metadata, Priority priority) {
+      Context context, Path path, FileArtifactValue metadata, Priority priority) {
     if (path.isSymbolicLink()) {
       try {
         path = path.getRelative(path.readSymbolicLink());
@@ -402,7 +429,7 @@ private Completable downloadFileNoCheckRx(
                         directExecutor())
                     .doOnComplete(
                         () -> {
-                          finalizeDownload(tempPath, finalPath);
+                          finalizeDownload(context, tempPath, finalPath);
                           completed.set(true);
                         }),
             tempPath -> {
@@ -439,11 +466,24 @@ public void downloadFile(Path path, FileArtifactValue metadata)
 
   protected ListenableFuture<Void> downloadFileAsync(
       PathFragment path, FileArtifactValue metadata, Priority priority) {
+    Context context = new Context();
     return toListenableFuture(
-        downloadFileRx(execRoot.getFileSystem().getPath(path), metadata, priority));
+        Completable.using(
+            () -> context,
+            ctx ->
+                downloadFileRx(context, execRoot.getFileSystem().getPath(path), metadata, priority),
+            Context::finalizeContext));
   }
 
-  private void finalizeDownload(Path tmpPath, Path path) throws IOException {
+  private void finalizeDownload(Context context, Path tmpPath, Path path) throws IOException {
+    Path parentDir = path.getParentDirectory();
+    // In case the parent directory of the destination is not writable, temporarily change it to
+    // writable. b/254844173.
+    if (parentDir != null && !parentDir.isWritable()) {
+      context.addNonWritableDir(parentDir);
+      parentDir.setWritable(true);
+    }
+
     // The permission of output file is changed to 0555 after action execution. We manually change
     // the permission here for the downloaded file to keep this behaviour consistent.
     tmpPath.chmod(0555);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -727,8 +727,10 @@ void updateActionCache(
       // Skyframe has already done all the filesystem access needed for outputs and swallows
       // IOExceptions for inputs. So an IOException is impossible here.
       throw new IllegalStateException(
-          "failed to update action cache for " + action.prettyPrint()
-              + ", but all outputs should already have been checked", e);
+          "failed to update action cache for "
+              + action.prettyPrint()
+              + ", but all outputs should already have been checked",
+          e);
     }
   }
 
@@ -866,12 +868,12 @@ void recordExecutionError() {
   }
 
   /**
-   * Returns true if the Builder is winding down (i.e. cancelling outstanding
-   * actions and preparing to abort.)
-   * The builder is winding down iff:
+   * Returns true if the Builder is winding down (i.e. cancelling outstanding actions and preparing
+   * to abort.) The builder is winding down iff:
+   *
    * <ul>
-   * <li>we had an execution error
-   * <li>we are not running with --keep_going
+   *   <li>we had an execution error
+   *   <li>we are not running with --keep_going
    * </ul>
    */
   private boolean isBuilderAborting() {
@@ -1197,8 +1199,10 @@ private ActionExecutionValue actuallyCompleteAction(
       Artifact primaryOutput = action.getPrimaryOutput();
       Path primaryOutputPath = actionExecutionContext.getInputPath(primaryOutput);
       try {
-        Preconditions.checkState(action.inputsDiscovered(),
-            "Action %s successfully executed, but inputs still not known", action);
+        Preconditions.checkState(
+            action.inputsDiscovered(),
+            "Action %s successfully executed, but inputs still not known",
+            action);
 
         try {
           flushActionFileSystem(actionExecutionContext.getActionFileSystem(), outputService);
@@ -1481,12 +1485,15 @@ private static void reportMissingOutputFile(
       String msg = prefix + "is a dangling symbolic link";
       reporter.handle(Event.error(action.getOwner().getLocation(), msg));
     } else {
-      String suffix = genrule ? " by genrule. This is probably "
-          + "because the genrule actually didn't create this output, or because the output was a "
-          + "directory and the genrule was run remotely (note that only the contents of "
-          + "declared file outputs are copied from genrules run remotely)" : "";
-      reporter.handle(Event.error(
-          action.getOwner().getLocation(), prefix + "was not created" + suffix));
+      String suffix =
+          genrule
+              ? " by genrule. This is probably because the genrule actually didn't create this"
+                  + " output, or because the output was a directory and the genrule was run"
+                  + " remotely (note that only the contents of declared file outputs are copied"
+                  + " from genrules run remotely)"
+              : "";
+      reporter.handle(
+          Event.error(action.getOwner().getLocation(), prefix + "was not created" + suffix));
     }
   }
 
@@ -1496,8 +1503,9 @@ private static void reportOutputTreeArtifactErrors(
     if (e instanceof FileNotFoundException) {
       errorMessage = String.format("TreeArtifact %s was not created", output.prettyPrint());
     } else {
-      errorMessage = String.format(
-          "Error while validating output TreeArtifact %s : %s", output, e.getMessage());
+      errorMessage =
+          String.format(
+              "Error while validating output TreeArtifact %s : %s", output, e.getMessage());
     }
 
     reporter.handle(Event.error(action.getOwner().getLocation(), errorMessage));
