Prevent failures creating output directories

I would receive reports of build failures along these lines:

> ERROR: some/path/BUILD:156:32: Executing genrule //some/path:some_target failed: failed to create output directory '.../some/path/foo'

In order to root cause this, I made a minor change to Bazel to extend its logging. See #17951. With that change applied, the error became:

> ERROR: some/path/BUILD:156:32: Executing genrule //some/path:some_target failed: failed to create output directory '.../some/path/foo': .../some/path/foo (Not a directory)

I was eventually able to reproduce this by manually creating an output file at bazel-bin/some/path, followed by attempting to build the target. It looks like Bazel simply attempts to create output directories without taking into consideration whether a file already exists at the target location. This is problematic when someone alters an existing build target to emit a directory instead of a regular file.

Note that this only an issue when the remote action file system is used.    Plain builds (ones that don't use Builds without the Bytes or    bb_clientd) are unaffected. This change addresses this issue by reusing    the same fallback path creation strategy that plain builds already use.

Closes #17968.

PiperOrigin-RevId: 523408378
Change-Id: I0d7559352687c317de72e0bf5bc6b5ba7452f97e

Author: EdSchouten

src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelper.java

@@ -61,10 +61,7 @@ private enum DirectoryState {
 
   /**
    * Creates output directories for an in-memory action file system ({@link
-   * com.google.devtools.build.lib.vfs.OutputService.ActionFileSystemType#inMemoryFileSystem}). The
-   * action-local filesystem starts empty, so we expect the output directory creation to always
-   * succeed. There can be no interference from state left behind by prior builds or other actions
-   * intra-build.
+   * com.google.devtools.build.lib.vfs.OutputService.ActionFileSystemType#inMemoryFileSystem}).
    */
   void createActionFsOutputDirectories(
       ImmutableSet<Artifact> actionOutputs, ArtifactPathResolver artifactPathResolver)
@@ -81,9 +78,13 @@ void createActionFsOutputDirectories(
       if (done.add(outputDir)) {
         try {
           outputDir.createDirectoryAndParents();
+          continue;
         } catch (IOException e) {
-          throw new CreateOutputDirectoryException(outputDir.asFragment(), e);
+          /* Fall through to plan B. */
         }
+
+        Path rootPath = artifactPathResolver.convertPath(outputFile.getRoot().getRoot().asPath());
+        forceCreateDirectoryAndParents(outputDir, rootPath);
       }
     }
   }
@@ -133,71 +134,77 @@ void createOutputDirectories(ImmutableSet<Artifact> actionOutputs)
       }
 
       if (done.add(outputDir)) {
+        Path rootPath = outputFile.getRoot().getRoot().asPath();
         try {
-          createAndCheckForSymlinks(outputDir, outputFile);
+          createAndCheckForSymlinks(outputDir, rootPath);
           continue;
         } catch (IOException e) {
           /* Fall through to plan B. */
         }
 
-        // Possibly some direct ancestors are not directories.  In that case, we traverse the
-        // ancestors downward, deleting any non-directories. This handles the case where a file
-        // becomes a directory. The traversal is done downward because otherwise we may delete
-        // files through a symlink in a parent directory. Since Blaze never creates such
-        // directories within a build, we have no idea where on disk we're actually deleting.
+        forceCreateDirectoryAndParents(outputDir, rootPath);
+      }
+    }
+  }
+
+  void forceCreateDirectoryAndParents(Path outputDir, Path rootPath)
+      throws CreateOutputDirectoryException {
+    // Possibly some direct ancestors are not directories.  In that case, we traverse the
+    // ancestors downward, deleting any non-directories. This handles the case where a file
+    // becomes a directory. The traversal is done downward because otherwise we may delete
+    // files through a symlink in a parent directory. Since Blaze never creates such
+    // directories within a build, we have no idea where on disk we're actually deleting.
+    //
+    // Symlinks should not be followed so in order to clean up symlinks pointing to Fileset
+    // outputs from previous builds. See bug [incremental build of Fileset fails if
+    // Fileset.out was changed to be a subdirectory of the old value].
+    try {
+      Path p = rootPath;
+      for (String segment : outputDir.relativeTo(p).segments()) {
+        p = p.getRelative(segment);
+
+        // This lock ensures that the only thread that observes a filesystem transition in
+        // which the path p first exists and then does not is the thread that calls
+        // p.delete() and causes the transition.
+        //
+        // If it were otherwise, then some thread A could test p.exists(), see that it does,
+        // then test p.isDirectory(), see that p isn't a directory (because, say, thread
+        // B deleted it), and then call p.delete(). That could result in two different kinds
+        // of failures:
+        //
+        // 1) In the time between when thread A sees that p is not a directory and when thread
+        // A calls p.delete(), thread B may reach the call to createDirectoryAndParents
+        // and create a directory at p, which thread A then deletes. Thread B would then try
+        // adding outputs to the directory it thought was there, and fail.
         //
-        // Symlinks should not be followed so in order to clean up symlinks pointing to Fileset
-        // outputs from previous builds. See bug [incremental build of Fileset fails if
-        // Fileset.out was changed to be a subdirectory of the old value].
+        // 2) In the time between when thread A sees that p is not a directory and when thread
+        // A calls p.delete(), thread B may create a directory at p, and then either create a
+        // subdirectory beneath it or add outputs to it. Then when thread A tries to delete p,
+        // it would fail.
+        Lock lock = outputDirectoryDeletionLock.get(p);
+        lock.lock();
         try {
-          Path p = outputFile.getRoot().getRoot().asPath();
-          for (String segment : outputDir.relativeTo(p).segments()) {
-            p = p.getRelative(segment);
-
-            // This lock ensures that the only thread that observes a filesystem transition in
-            // which the path p first exists and then does not is the thread that calls
-            // p.delete() and causes the transition.
-            //
-            // If it were otherwise, then some thread A could test p.exists(), see that it does,
-            // then test p.isDirectory(), see that p isn't a directory (because, say, thread
-            // B deleted it), and then call p.delete(). That could result in two different kinds
-            // of failures:
-            //
-            // 1) In the time between when thread A sees that p is not a directory and when thread
-            // A calls p.delete(), thread B may reach the call to createDirectoryAndParents
-            // and create a directory at p, which thread A then deletes. Thread B would then try
-            // adding outputs to the directory it thought was there, and fail.
-            //
-            // 2) In the time between when thread A sees that p is not a directory and when thread
-            // A calls p.delete(), thread B may create a directory at p, and then either create a
-            // subdirectory beneath it or add outputs to it. Then when thread A tries to delete p,
-            // it would fail.
-            Lock lock = outputDirectoryDeletionLock.get(p);
-            lock.lock();
-            try {
-              FileStatus stat = p.statIfFound(Symlinks.NOFOLLOW);
-              if (stat == null) {
-                // Missing entry: Break out and create expected directories.
-                break;
-              }
-              if (stat.isDirectory()) {
-                // If this directory used to be a tree artifact it won't be writable.
-                p.setWritable(true);
-                knownDirectories.put(p.asFragment(), DirectoryState.FOUND);
-              } else {
-                // p may be a file or symlink (possibly from a Fileset in a previous build).
-                p.delete(); // throws IOException
-                break;
-              }
-            } finally {
-              lock.unlock();
-            }
+          FileStatus stat = p.statIfFound(Symlinks.NOFOLLOW);
+          if (stat == null) {
+            // Missing entry: Break out and create expected directories.
+            break;
           }
-          outputDir.createDirectoryAndParents();
-        } catch (IOException e) {
-          throw new CreateOutputDirectoryException(outputDir.asFragment(), e);
+          if (stat.isDirectory()) {
+            // If this directory used to be a tree artifact it won't be writable.
+            p.setWritable(true);
+            knownDirectories.put(p.asFragment(), DirectoryState.FOUND);
+          } else {
+            // p may be a file or symlink (possibly from a Fileset in a previous build).
+            p.delete(); // throws IOException
+            break;
+          }
+        } finally {
+          lock.unlock();
         }
       }
+      outputDir.createDirectoryAndParents();
+    } catch (IOException e) {
+      throw new CreateOutputDirectoryException(outputDir.asFragment(), e);
     }
   }
 
@@ -206,8 +213,7 @@ void createOutputDirectories(ImmutableSet<Artifact> actionOutputs)
    * output file. These are all expected to be regular directories. Violations of this expectations
    * can only come from state left behind by previous invocations or external filesystem mutation.
    */
-  private void createAndCheckForSymlinks(Path dir, Artifact outputFile) throws IOException {
-    Path rootPath = outputFile.getRoot().getRoot().asPath();
+  private void createAndCheckForSymlinks(Path dir, Path rootPath) throws IOException {
     PathFragment root = rootPath.asFragment();
 
     // If the output root has not been created yet, do so now.

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

@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.standalone.StandaloneModule;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
 import org.junit.After;
 import org.junit.Test;
@@ -432,6 +433,37 @@ public void symlinkToNestedDirectory() throws Exception {
     buildTarget("//a:one_local", "//a:two_local", "//a:one_remote", "//a:two_remote");
   }
 
+  @Test
+  public void replaceOutputDirectoryWithFile() throws Exception {
+    write(
+        "a/defs.bzl",
+        "def _impl(ctx):",
+        "  dir = ctx.actions.declare_directory(ctx.label.name + '.dir')",
+        "  ctx.actions.run_shell(",
+        "    outputs = [dir],",
+        "    command = 'touch $1/hello',",
+        "    arguments = [dir.path],",
+        "  )",
+        "  return DefaultInfo(files = depset([dir]))",
+        "",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        ")");
+    write("a/BUILD", "load(':defs.bzl', 'my_rule')", "", "my_rule(name = 'hello')");
+
+    setDownloadToplevel();
+    buildTarget("//a:hello");
+
+    // Replace the existing output directory of the package with a file.
+    // A subsequent build should remove this file and replace it with a
+    // directory.
+    Path outputPath = getOutputPath("a");
+    outputPath.deleteTree();
+    FileSystemUtils.writeContent(outputPath, new byte[] {1, 2, 3, 4, 5});
+
+    buildTarget("//a:hello");
+  }
+
   @Test
   public void remoteCacheEvictBlobs_whenPrefetchingInput_exitWithCode39() throws Exception {
     // Arrange: Prepare workspace and populate remote cache