Skip to content

Commit

Permalink
Don't download all artifacts with BwoB when their TTL expires
Browse files Browse the repository at this point in the history 
The TLL governs the lifetime of the remote metadata, but shouldn't be itself result in files being downloaded that otherwise wouldn't be (e.g., with `--remote_download_minimal`). This fixes a bug that causes warm Bazel servers with default settings apart from `--remote_download_minimal` to start downloading all top-level artifacts after three hours.
  • Loading branch information
@fmeum
fmeum committed Feb 27, 2025
1 parent 46af31c commit ddc222f
Show file tree
Hide file tree
Showing 8 changed files with 171 additions and 34 deletions.
6 changes: 3 additions & 3 deletions src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ private static boolean shouldTrustMetadata(
if (outputChecker == null || metadata == null) {
return true;
}
return outputChecker.shouldTrustArtifact(artifact, metadata);
return outputChecker.shouldTrustMetadata(artifact, metadata);
}

private static boolean shouldTrustTreeMetadata(
Expand All @@ -262,15 +262,15 @@ private static boolean shouldTrustTreeMetadata(
.getArchivedRepresentation()
.map(ArchivedRepresentation::archivedFileValue)
.orElseThrow();
if (!outputChecker.shouldTrustArtifact(archivedArtifact, archivedMetadata)) {
if (!outputChecker.shouldTrustMetadata(archivedArtifact, archivedMetadata)) {
return false;
}
}
for (Map.Entry<TreeFileArtifact, FileArtifactValue> entry :
treeMetadata.getChildValues().entrySet()) {
TreeFileArtifact child = entry.getKey();
FileArtifactValue childMetadata = entry.getValue();
if (!outputChecker.shouldTrustArtifact(child, childMetadata)) {
if (!outputChecker.shouldTrustMetadata(child, childMetadata)) {
return false;
}
}
Expand Down
12 changes: 8 additions & 4 deletions src/main/java/com/google/devtools/build/lib/actions/OutputChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,18 @@
package com.google.devtools.build.lib.actions;

/**
* An interface used to check whether the metadata for an output should be trusted.
*
* <p>Used to invalidate metadata when the respective contents are stored with a bounded lifetime.
* An interface used to check whether an output should be downloaded or its metadata revalidated
* when it is stored with a bounded lifetime.
*/
public interface OutputChecker {
static final OutputChecker TRUST_ALL = (file, metadata) -> true;
static final OutputChecker TRUST_LOCAL_ONLY = (file, metadata) -> !metadata.isRemote();

/** Returns whether the given output should be downloaded. */
default boolean shouldDownloadOutput(ActionInput output, FileArtifactValue metadata) {
return !shouldTrustMetadata(output, metadata);
}

/** Returns whether the given metadata should be trusted. */
boolean shouldTrustArtifact(ActionInput file, FileArtifactValue metadata);
boolean shouldTrustMetadata(ActionInput file, FileArtifactValue metadata);
}
18 changes: 10 additions & 8 deletions src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -724,15 +724,17 @@ public void finalizeAction(Action action, OutputMetadataStore outputMetadataStor
}

if (output.isTreeArtifact()) {
var children =
outputMetadataStore.getTreeArtifactValue((SpecialArtifact) output).getChildren();
for (var file : children) {
if (remoteOutputChecker.shouldDownloadOutput(file)) {
outputsToDownload.add(file);
}
}
outputMetadataStore
.getTreeArtifactValue((SpecialArtifact) output)
.getChildValues()
.forEach(
(child, childMetadata) -> {
if (remoteOutputChecker.shouldDownloadOutput(child, childMetadata)) {
outputsToDownload.add(child);
}
});
} else {
if (remoteOutputChecker.shouldDownloadOutput(output)) {
if (remoteOutputChecker.shouldDownloadOutput(output, metadata)) {
outputsToDownload.add(output);
}
}
Expand Down
18 changes: 11 additions & 7 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@
import java.util.regex.Pattern;
import javax.annotation.Nullable;

/** An {@link OutputChecker} that checks the TTL of remote metadata. */
/**
* An {@link OutputChecker} that checks the TTL of remote metadata and decides which outputs to
* download.
*/
public class RemoteOutputChecker implements OutputChecker {
private enum CommandMode {
UNKNOWN,
Expand Down Expand Up @@ -272,22 +275,23 @@ private boolean matchesPattern(PathFragment execPath) {
}

/** Returns whether this {@link ActionInput} should be downloaded. */
public boolean shouldDownloadOutput(ActionInput output) {
@Override
public boolean shouldDownloadOutput(ActionInput output, FileArtifactValue metadata) {
checkState(
!(output instanceof Artifact && ((Artifact) output).isTreeArtifact()),
"shouldDownloadOutput should not be called on a tree artifact");
return shouldDownloadOutput(output.getExecPath());
return metadata.isRemote() && shouldDownloadOutput(output.getExecPath());
}

/** Returns whether an {@link ActionInput} with the given path should be downloaded. */
/** Returns whether a remote {@link ActionInput} with the given path should be downloaded. */
public boolean shouldDownloadOutput(PathFragment execPath) {
return outputsMode == RemoteOutputsMode.ALL
|| pathsToDownload.contains(execPath)
|| matchesPattern(execPath);
}

@Override
public boolean shouldTrustArtifact(ActionInput file, FileArtifactValue metadata) {
public boolean shouldTrustMetadata(ActionInput file, FileArtifactValue metadata) {
// Local metadata is always trusted.
if (!metadata.isRemote()) {
return true;
Expand All @@ -300,12 +304,12 @@ public boolean shouldTrustArtifact(ActionInput file, FileArtifactValue metadata)
if (lastRemoteOutputChecker != null) {
// This is an incremental build. If the file was downloaded by previous build and is now
// missing, invalidate the action.
if (lastRemoteOutputChecker.shouldDownloadOutput(file)) {
if (lastRemoteOutputChecker.shouldDownloadOutput(file, metadata)) {
return false;
}
}

if (shouldDownloadOutput(file)) {
if (shouldDownloadOutput(file, metadata)) {
return false;
}

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ private void downloadArtifact(
for (var child : treeMetadata.getChildValues().entrySet()) {
var treeFile = child.getKey();
var metadata = child.getValue();
if (!outputChecker.shouldTrustArtifact(treeFile, metadata)) {
if (outputChecker.shouldDownloadOutput(treeFile, metadata)) {
filesToDownload.add(treeFile);
}
}
Expand All @@ -473,7 +473,7 @@ private void downloadArtifact(
return;
}

if (!outputChecker.shouldTrustArtifact(artifact, metadata)) {
if (outputChecker.shouldDownloadOutput(artifact, metadata)) {
var action =
ActionUtils.getActionForLookupData(env, derivedArtifact.getGeneratingActionKey());
var future =
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ private boolean artifactIsDirtyWithDirectSystemCalls(
boolean isTrustedRemoteValue =
fileMetadata.getType() == FileStateType.NONEXISTENT
&& lastKnownData.isRemote()
&& outputChecker.shouldTrustArtifact(file, lastKnownData);
&& outputChecker.shouldTrustMetadata(file, lastKnownData);
if (!isTrustedRemoteValue && fileMetadata.couldBeModifiedSince(lastKnownData)) {
modifiedOutputsReceiver.reportModifiedOutputFile(
fileMetadata.getType() != FileStateType.NONEXISTENT
Expand Down
16 changes: 8 additions & 8 deletions src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,7 @@ public void saveOutputMetadata_localMetadataIsDifferentFromRemoteMetadata_notCac
/* remoteDefaultPlatformProperties= */ ImmutableMap.of(),
outputChecker);
verify(outputChecker)
.shouldTrustArtifact(argThat(arg -> arg.getExecPathString().endsWith("bin/dummy")), any());
.shouldTrustMetadata(argThat(arg -> arg.getExecPathString().endsWith("bin/dummy")), any());
// Not cached since local file changed
runAction(
action,
Expand Down Expand Up @@ -787,7 +787,7 @@ public void saveOutputMetadata_untrustedRemoteMetadataFromOutputStore_notCached(
fakeOutputMetadataStore.injectFile(output, metadata);

OutputChecker outputChecker = mock(OutputChecker.class);
when(outputChecker.shouldTrustArtifact(any(), any())).thenReturn(false);
when(outputChecker.shouldTrustMetadata(any(), any())).thenReturn(false);

runAction(
action,
Expand Down Expand Up @@ -1072,7 +1072,7 @@ public void saveOutputMetadata_treeMetadata_localFileMetadataLoaded() throws Exc
runAction(action);
writeIsoLatin1(output.getPath().getRelative("file2"), "modified_local");
var outputChecker = mock(OutputChecker.class);
when(outputChecker.shouldTrustArtifact(any(), any())).thenReturn(true);
when(outputChecker.shouldTrustMetadata(any(), any())).thenReturn(true);
var token =
cacheChecker.getTokenIfNeedToExecute(
action,
Expand All @@ -1086,9 +1086,9 @@ public void saveOutputMetadata_treeMetadata_localFileMetadataLoaded() throws Exc
/* remoteDefaultPlatformProperties= */ ImmutableMap.of(),
outputChecker);
verify(outputChecker)
.shouldTrustArtifact(argThat(arg -> arg.getExecPathString().endsWith("file1")), any());
.shouldTrustMetadata(argThat(arg -> arg.getExecPathString().endsWith("file1")), any());
verify(outputChecker)
.shouldTrustArtifact(argThat(arg -> arg.getExecPathString().endsWith("file2")), any());
.shouldTrustMetadata(argThat(arg -> arg.getExecPathString().endsWith("file2")), any());
// Not cached since local file changed
runAction(
action,
Expand Down Expand Up @@ -1143,7 +1143,7 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws
runAction(action);
writeIsoLatin1(ArchivedTreeArtifact.createForTree(output).getPath(), "modified");
var outputChecker = mock(OutputChecker.class);
when(outputChecker.shouldTrustArtifact(any(), any())).thenReturn(true);
when(outputChecker.shouldTrustMetadata(any(), any())).thenReturn(true);
var token =
cacheChecker.getTokenIfNeedToExecute(
action,
Expand All @@ -1156,7 +1156,7 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws
/* artifactExpander= */ null,
/* remoteDefaultPlatformProperties= */ ImmutableMap.of(),
outputChecker);
when(outputChecker.shouldTrustArtifact(any(), any())).thenReturn(true);
when(outputChecker.shouldTrustMetadata(any(), any())).thenReturn(true);
// Not cached since local file changed
runAction(
action,
Expand Down Expand Up @@ -1439,7 +1439,7 @@ public void saveOutputMetadata_untrustedRemoteTreeMetadataFromOutputStore_notCac
fakeOutputMetadataStore.injectTree(tree, treeMetadata);

OutputChecker outputChecker = mock(OutputChecker.class);
when(outputChecker.shouldTrustArtifact(any(), any())).thenReturn(false);
when(outputChecker.shouldTrustMetadata(any(), any())).thenReturn(false);

runAction(
action,
Expand Down
129 changes: 128 additions & 1 deletion ...st/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.eventbus.Subscribe;
import com.google.devtools.build.lib.actions.ActionExecutedEvent;
import com.google.devtools.build.lib.actions.Artifact;
Expand Down Expand Up @@ -1663,7 +1664,70 @@ public void remoteFilesExpiredBetweenBuilds_rerunGeneratingActions() throws Exce
}

@Test
public void remoteTreeFilesExpiredBetweenBuilds_rerunGeneratingActions() throws Exception {
public void remoteFilesExpiredBetweenBuilds_rerunGeneratingActions_notDownloadedWithMinimal()
throws Exception {
// Arrange: Prepare workspace and populate remote cache
addOptions("--experimental_remote_cache_eviction_retries=0");
write(
"a/BUILD",
"""
genrule(
name = "foo",
srcs = ["foo.in"],
outs = ["foo.out"],
cmd = "cat $(SRCS) > $@",
)
genrule(
name = "bar",
srcs = [
"foo.out",
"bar.in",
],
outs = ["bar.out"],
cmd = "cat $(SRCS) > $@",
)
""");
write("a/foo.in", "foo");
write("a/bar.in", "bar");

// Populate remote cache
buildTarget("//a:bar");
assertOutputDoesNotExist("a/foo.out");
assertOutputDoesNotExist("a/bar.out");
getOutputBase().getRelative("action_cache").deleteTreesBelow();
restartServer();

// Clean build, foo.out and bar.out aren't downloaded
addOptions("--experimental_remote_cache_ttl=0s");
buildTarget("//a:bar");
assertOutputDoesNotExist("a/foo.out");
assertOutputDoesNotExist("a/bar.out");

// Evict blobs from remote cache
evictAllBlobs();

// Act: Do an incremental build
write("a/bar.in", "updated bar");
buildTarget("//a:bar");
waitDownloads();

// Assert: target was successfully built
assertOutputDoesNotExist("a/foo.out");
assertOutputDoesNotExist("a/bar.out");
var metadata = Iterables.getOnlyElement(getMetadata("//a:bar").values());
assertThat(metadata.isRemote()).isTrue();
assertThat(metadata.getDigest())
.isEqualTo(
getDigestHashFunction()
.getHashFunction()
.hashString("foo" + lineSeparator() + "updated bar" + lineSeparator(), UTF_8)
.asBytes());
}

@Test
public void remoteTreeFilesExpiredBetweenBuilds_rerunGeneratingActions_notDownloadedWithMinimal()
throws Exception {
// Arrange: Prepare workspace and populate remote cache
write("BUILD");
writeOutputDirRule();
Expand Down Expand Up @@ -1715,6 +1779,69 @@ public void remoteTreeFilesExpiredBetweenBuilds_rerunGeneratingActions() throws
assertValidOutputFile("a/bar.out", "file-inside\nupdated bar" + lineSeparator());
}

@Test
public void remoteTreeFilesExpiredBetweenBuilds_rerunGeneratingActions() throws Exception {
// Arrange: Prepare workspace and populate remote cache
addOptions("--experimental_remote_cache_eviction_retries=0");
write("BUILD");
writeOutputDirRule();
write(
"a/BUILD",
"""
load("//:output_dir.bzl", "output_dir")
output_dir(
name = "foo.out",
content_map = {"file-inside": "hello world"},
)
genrule(
name = "bar",
srcs = [
"foo.out",
"bar.in",
],
outs = ["bar.out"],
cmd = "( ls $(location :foo.out); cat $(location :bar.in) ) > $@",
)
""");
write("a/bar.in", "bar");

// Populate remote cache
buildTarget("//a:bar");
assertThat(getOutputPath("a/foo.out").getDirectoryEntries()).isEmpty();
assertOutputDoesNotExist("a/bar.out");
getOutputBase().getRelative("action_cache").deleteTreesBelow();
restartServer();

// Clean build, foo.out and bar.out aren't downloaded
addOptions("--experimental_remote_cache_ttl=0s");
buildTarget("//a:bar");
assertOutputDoesNotExist("a/foo.out/file-inside");
assertOutputDoesNotExist("a/bar.out");

// Evict blobs from remote cache
evictAllBlobs();

// Act: Do an incremental build
write("a/bar.in", "updated bar");
// Also request foo.out to verify that it's files aren't downloaded.
buildTarget("//a:bar", "//a:foo.out");
waitDownloads();

// Assert: target was successfully built
assertOutputDoesNotExist("a/foo.out/file-inside");
assertOutputDoesNotExist("a/bar.out");
var metadata = Iterables.getOnlyElement(getMetadata("//a:bar").values());
assertThat(metadata.isRemote()).isTrue();
assertThat(metadata.getDigest())
.isEqualTo(
getDigestHashFunction()
.getHashFunction()
.hashString("file-inside\nupdated bar" + lineSeparator(), UTF_8)
.asBytes());
}

@Test
public void nonDeclaredSymlinksFromLocalActions() throws Exception {
write(
Expand Down

0 comments on commit ddc222f

Please sign in to comment.