Skip to content

Fix concurrent TreeArtifact prefetches #29803

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

Open
tamird wants to merge 2 commits into
base: master
Choose a base branch
from
+302 −62
Open

Fix concurrent TreeArtifact prefetches #29803

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
d702bf5
Fix concurrent TreeArtifact prefetches
tamird Jun 10, 2026
8c5acce
Allow concurrent TreeArtifact finalization
tamird Jun 10, 2026
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
161 changes: 99 additions & 62 deletions src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,14 @@
import static com.google.devtools.build.lib.remote.util.Utils.mergeBulkTransfer;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.common.collect.SetMultimap;
import com.google.common.flogger.GoogleLogger;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.Striped;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
import com.google.devtools.build.lib.actions.ActionInput;
Expand Down Expand Up @@ -60,10 +63,11 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.function.Supplier;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -206,6 +210,12 @@ void setOutputPermissions(Path dir) throws IOException {

private final DirectoryTracker directoryTracker = new DirectoryTracker();

// TreeArtifact downloads use temporary paths. Coordinate finalization and permission restoration
// by resolved tree root. Weak locks over the full hash space allow unrelated trees to finalize
// concurrently without retaining every lock for the lifetime of the prefetcher.
private final Striped<ReadWriteLock> treeArtifactLocks =
Striped.lazyWeakReadWriteLock(Integer.MAX_VALUE);

/** A symlink in the output tree that points to another artifact's absolute path. */
record Symlink(Path linkPath, Path targetPath) {
Symlink {
Expand Down Expand Up @@ -346,21 +356,18 @@ public ListenableFuture<Void> prefetchFilesInterruptibly(
return immediateVoidFuture();
}

// Collect the set of directories whose output permissions must be set at the end of this call.
// This responsibility cannot lie with the downloading of an individual file, because multiple
// files may be concurrently downloaded into the same directory within a single call to
// prefetchFiles, and two concurrent calls to prefetchFiles may prefetch the same file. In the
// latter case, the second call will have its downloads deduplicated against the first call, but
// it must still synchronize on the output permissions having been set.
Set<Path> dirsWithOutputPermissions = Sets.newConcurrentHashSet();
// Collect directories whose output permissions must be restored at the end of this call,
// grouped by tree root for synchronization. Keeping restoration call-scoped avoids toggling
// shared ancestors between child finalizations. A concurrent call for the same child may join
// the cached download but must still participate in permission restoration before returning.
SetMultimap<Path, Path> directoriesByTreeRoot = HashMultimap.create();

// Using plain futures to avoid RxJava overheads.
List<ListenableFuture<Void>> transfers = new ArrayList<>(files.size());
try (var s = Profiler.instance().profile("compose prefetches")) {
for (var file : files) {
transfers.add(
prefetchFile(
action, dirsWithOutputPermissions, metadataSupplier, file, priority, reason));
prefetchFile(action, directoriesByTreeRoot, metadataSupplier, file, priority, reason));
}
}

Expand All @@ -373,10 +380,19 @@ public ListenableFuture<Void> prefetchFilesInterruptibly(
mergedTransfer,
unused -> {
try {
// Set output permissions on tree artifact subdirectories, matching the behavior of
// SkyframeActionExecutor#checkOutputs for artifacts produced by local actions.
for (Path dir : dirsWithOutputPermissions) {
directoryTracker.setOutputPermissions(dir);
// Match the directory output permissions set by SkyframeActionExecutor#checkOutputs.
// Take the root write lock so restoration cannot make an ancestor read-only while any
// call holds a read lock to move a child into the tree.
for (var entry : directoriesByTreeRoot.asMap().entrySet()) {
Lock lock = treeArtifactLocks.get(entry.getKey().asFragment()).writeLock();
lock.lock();

@fmeum fmeum Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to make this a rw lock? Moves could happen concurrently (reads), only permission changes must be exclusive (write).

@tamird tamird Jun 10, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 8c5acce. Finalization holds a read lock from reopening ancestors through the move, while call-scoped permission restoration takes the write lock. Widening permissions is safe under read locks because it is monotonic during finalization and DirectoryTracker.compute serializes each directory transition. The deterministic test also verifies that one move completes while another is paused, but restoration cannot finish.

try {
for (Path dir : entry.getValue()) {
directoryTracker.setOutputPermissions(dir);
}
} finally {
lock.unlock();
}
}
} catch (IOException e) {
return immediateFailedFuture(e);
Expand All @@ -388,7 +404,7 @@ public ListenableFuture<Void> prefetchFilesInterruptibly(

private ListenableFuture<Void> prefetchFile(
@Nullable ActionExecutionMetadata action,
Set<Path> dirsWithOutputPermissions,
SetMultimap<Path, Path> directoriesByTreeRoot,
MetadataSupplier metadataSupplier,
ActionInput input,
Priority priority,
Expand Down Expand Up @@ -436,7 +452,7 @@ private ListenableFuture<Void> prefetchFile(
input,
inputPath,
treeRootPath,
dirsWithOutputPermissions,
directoriesByTreeRoot,
input,
metadata,
priority,
Expand Down Expand Up @@ -560,7 +576,7 @@ private Completable downloadFileNoCheckRx(
ActionInput input,
Path path,
@Nullable Path treeRoot,
Set<Path> dirsWithOutputPermissions,
SetMultimap<Path, Path> directoriesByTreeRoot,
ActionInput actionInput,
FileArtifactValue metadata,
Priority priority,
Expand All @@ -581,24 +597,27 @@ private Completable downloadFileNoCheckRx(
return Completable.error(e);
}

// Downloads are written to the actual host file system, not any overlays.
Path finalPath = path.forHostFileSystem();

ImmutableList.Builder<Path> dirsWithOutputPermissions = ImmutableList.builder();
if (treeRoot != null
&& actionInput instanceof Artifact artifact
&& artifact.isChildOfDeclaredDirectory()) {
// Arrange for the output permissions to be set on every directory inside the tree artifact.
// This must be done at assembly time to ensure that the permissions are set before the
// prefetchFiles call returns, even when the actual downloads are deduplicated against a
// concurrent call. See finalizeDownload for why we don't do so in other cases.
for (Path dir = path.getParentDirectory();
dir.startsWith(treeRoot);
Path finalTreeRoot = treeRoot.forHostFileSystem();
// Record directories child-to-root. Every ancestor must be explicitly reopened because
// ActionOutputDirectoryHelper caches existing directories and may otherwise leave a
// previously restored ancestor read-only.
for (Path dir = finalPath.getParentDirectory();
dir.startsWith(finalTreeRoot);
dir = dir.getParentDirectory()) {
if (!dirsWithOutputPermissions.add(dir)) {
break;
}
dirsWithOutputPermissions.add(dir);
}
}

// Downloads should always be written to the "actual" host file system, not any overlays.
Path finalPath = path.forHostFileSystem();
ImmutableList<Path> directories = dirsWithOutputPermissions.build();
if (!directories.isEmpty()) {
directoriesByTreeRoot.putAll(directories.getLast(), directories);
}

Completable download =
usingTempPath(
Expand All @@ -617,10 +636,7 @@ private Completable downloadFileNoCheckRx(
.doOnComplete(
() -> {
finalizeDownload(
metadata,
tempPath.forHostFileSystem(),
finalPath,
dirsWithOutputPermissions);
metadata, tempPath.forHostFileSystem(), finalPath, directories);
alreadyDeleted.set(true);
}));

Expand All @@ -637,40 +653,61 @@ private Completable downloadFileNoCheckRx(
}

private void finalizeDownload(
FileArtifactValue metadata, Path tmpPath, Path finalPath, Set<Path> dirsWithOutputPermissions)
FileArtifactValue metadata,
Path tmpPath,
Path finalPath,
ImmutableList<Path> dirsWithOutputPermissions)
throws IOException {
// Set file output permissions, matching SkyframeActionExecutor#checkOutputs for artifacts
// produced by local actions. The temporary path is outside the output tree, so this can happen
// before entering the TreeArtifact root's critical section.
tmpPath.chmod(outputPermissions.getPermissionsMode());

Path parentDir = checkNotNull(finalPath.getParentDirectory());
// A read lock allows concurrent child moves, while preventing a prefetch call from restoring
// the tree's output permissions until every move in progress has completed.
@Nullable Lock treeArtifactLock =
dirsWithOutputPermissions.isEmpty()
? null
: treeArtifactLocks
.get(dirsWithOutputPermissions.getLast().asFragment())
.readLock();
if (treeArtifactLock != null) {
treeArtifactLock.lock();
}

// Compare as fragments since execRoot may be located on a file system overlaying the host
// file system where the download is written to.
if (finalPath.asFragment().startsWith(execRoot.asFragment())) {
// Ensure the parent directory exists and is writable. We cannot rely on this precondition to
// have been established by the execution of the owning action in a previous invocation, since
// the output tree may have been externally modified in between invocations.
if (dirsWithOutputPermissions.contains(parentDir)) {
// The file belongs to a tree artifact created by an action that declared an output
// directory (as opposed to an action template expansion). The output permissions should be
// set on the parent directory after prefetching.
directoryTracker.setTemporarilyWritable(parentDir);
try {
// Compare as fragments since execRoot may be located on a file system overlaying the host
// file system where the download is written to.
if (finalPath.asFragment().startsWith(execRoot.asFragment())) {
// Ensure the parent directory exists and is writable. We cannot rely on this precondition
// to have been established by the execution of the owning action in a previous invocation,
// since the output tree may have been externally modified in between invocations.
if (!dirsWithOutputPermissions.isEmpty()) {
for (Path dir : dirsWithOutputPermissions.reverse()) {
directoryTracker.setTemporarilyWritable(dir);
}
} else {
// One of the following must apply:
// (1) The file does not belong to a tree artifact.
// (2) The file belongs to a tree artifact created by an action template expansion.
// In case (1), the parent directory is a package or a subdirectory of a package, and
// should remain writable. In case (2), even though we arguably ought to set the output
// permissions on the parent directory to match local execution, we choose not to do it
// and avoid the additional implementation complexity required to detect a race condition
// between concurrent calls touching the same directory.
directoryTracker.setPermanentlyWritable(parentDir);
}
} else {
// One of the following must apply:
// (1) The file does not belong to a tree artifact.
// (2) The file belongs to a tree artifact created by an action template expansion.
// In case (1), the parent directory is a package or a subdirectory of a package, and should
// remain writable. In case (2), even though we arguably ought to set the output permissions
// on the parent directory to match local execution, we choose not to do it and avoid the
// additional implementation complexity required to detect a race condition between
// concurrent calls touching the same directory.
directoryTracker.setPermanentlyWritable(parentDir);
parentDir.createDirectoryAndParents();
}
} else {
parentDir.createDirectoryAndParents();
}

// Set output permissions on files, matching the behavior of SkyframeActionExecutor#checkOutputs
// for artifacts produced by local actions.
tmpPath.chmod(outputPermissions.getPermissionsMode());
FileSystemUtils.moveFile(tmpPath, finalPath);
FileSystemUtils.moveFile(tmpPath, finalPath);
} finally {
if (treeArtifactLock != null) {
treeArtifactLock.unlock();
}
}

// Set the contents proxy when supported, to make future modification checks cheaper.
metadata.setContentsProxy(FileContentsProxy.create(finalPath.stat()));
Expand Down
Loading