Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.2.0]Prevent failures creating output directories #18062

Merged
merged 2 commits into from
Apr 13, 2023
Merged

[6.2.0]Prevent failures creating output directories #18062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e028977
Prevent failures creating output directories
EdSchouten Apr 11, 2023
234d3f2
Merge branch 'release-6.2.0' into ks/cherry-pick17968
ShreeM01 Apr 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 67 additions & 61 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelper.java
View file
Open in desktop
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
Open in desktop
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 @@ -423,6 +424,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