Skip to content

Commit

Permalink
Prevent failures creating output directories
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@EdSchouten @copybara-github
EdSchouten authored and copybara-github committed Apr 11, 2023
1 parent 088d8e9 commit 4050120
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 61 deletions.
128 changes: 67 additions & 61 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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);
}
}
}
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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.
Expand Down
32 changes: 32 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 4050120

Please sign in to comment.