From 405012066d2394ad9986830ca028badd3c65ad38 Mon Sep 17 00:00:00 2001 From: Ed Schouten Date: Tue, 11 Apr 2023 08:26:49 -0700 Subject: [PATCH] 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 https://github.com/bazelbuild/bazel/pull/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 --- .../skyframe/ActionOutputDirectoryHelper.java | 128 +++++++++--------- .../BuildWithoutTheBytesIntegrationTest.java | 32 +++++ 2 files changed, 99 insertions(+), 61 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelper.java index 6fd2c88ab7cd21..17391ce6b070d5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelper.java +++ b/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 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 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 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. diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java index 7f8429707f088f..1922c99c9a674d 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java +++ b/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