Remote: Don't load remote metadata from action cache if remote cache is disabled.
Part of #14252.
Closes #14252.
PiperOrigin-RevId: 410448656
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
index 8dfe747..312e0c3 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
@@ -414,7 +414,8 @@
EventHandler handler,
MetadataHandler metadataHandler,
ArtifactExpander artifactExpander,
- Map<String, String> remoteDefaultPlatformProperties)
+ Map<String, String> remoteDefaultPlatformProperties,
+ boolean isRemoteCacheEnabled)
throws InterruptedException {
// TODO(bazel-team): (2010) For RunfilesAction/SymlinkAction and similar actions that
// produce only symlinks we should not check whether inputs are valid at all - all that matters
@@ -456,8 +457,11 @@
}
ActionCache.Entry entry = getCacheEntry(action);
CachedOutputMetadata cachedOutputMetadata = null;
- // load remote metadata from action cache
- if (entry != null && !entry.isCorrupted() && cacheConfig.storeOutputMetadata()) {
+ if (entry != null
+ && !entry.isCorrupted()
+ && cacheConfig.storeOutputMetadata()
+ && isRemoteCacheEnabled) {
+ // load remote metadata from action cache
cachedOutputMetadata = loadCachedOutputMetadata(action, entry, metadataHandler);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index 7a0b0df..c42189c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -599,6 +599,7 @@
remoteOptions != null
? remoteOptions.getRemoteDefaultExecProperties()
: ImmutableSortedMap.of();
+ boolean isRemoteCacheEnabled = remoteOptions != null && remoteOptions.isRemoteCacheEnabled();
token =
actionCacheChecker.getTokenIfNeedToExecute(
action,
@@ -609,7 +610,8 @@
: null,
metadataHandler,
artifactExpander,
- remoteDefaultProperties);
+ remoteDefaultProperties,
+ isRemoteCacheEnabled);
} catch (UserExecException e) {
throw e.toActionExecutionException(action);
}
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
index 00e67ad..536d086 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
@@ -183,7 +183,8 @@
/*handler=*/ null,
metadataHandler,
/*artifactExpander=*/ null,
- platform);
+ platform,
+ /*isRemoteCacheEnabled=*/ true);
if (token != null) {
// Real action execution would happen here.
ActionExecutionContext context = mock(ActionExecutionContext.class);
@@ -440,7 +441,8 @@
/*handler=*/ null,
new FakeMetadataHandler(),
/*artifactExpander=*/ null,
- /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()))
+ /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
+ /*isRemoteCacheEnabled=*/ true))
.isNotNull();
}
@@ -557,7 +559,8 @@
/*handler=*/ null,
metadataHandler,
/*artifactExpander=*/ null,
- /*remoteDefaultPlatformProperties=*/ ImmutableMap.of());
+ /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
+ /*isRemoteCacheEnabled=*/ true);
assertThat(output.getPath().exists()).isFalse();
assertThat(token).isNull();
@@ -568,6 +571,33 @@
}
@Test
+ public void saveOutputMetadata_remoteOutputUnavailable_remoteFileMetadataNotLoaded()
+ throws Exception {
+ cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true);
+ Artifact output = createArtifact(artifactRoot, "bin/dummy");
+ String content = "content";
+ Action action = new InjectOutputFileMetadataAction(output, createRemoteFileMetadata(content));
+ MetadataHandler metadataHandler = new FakeMetadataHandler();
+
+ runAction(action);
+ Token token =
+ cacheChecker.getTokenIfNeedToExecute(
+ action,
+ /*resolvedCacheArtifacts=*/ null,
+ /*clientEnv=*/ ImmutableMap.of(),
+ /*handler=*/ null,
+ metadataHandler,
+ /*artifactExpander=*/ null,
+ /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
+ /*isRemoteCacheEnabled=*/ false);
+
+ assertThat(output.getPath().exists()).isFalse();
+ assertThat(token).isNotNull();
+ ActionCache.Entry entry = cache.get(output.getExecPathString());
+ assertThat(entry).isNull();
+ }
+
+ @Test
public void saveOutputMetadata_localMetadataIsSameAsRemoteMetadata_cached() throws Exception {
cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true);
Artifact output = createArtifact(artifactRoot, "bin/dummy");
@@ -679,7 +709,8 @@
/*handler=*/ null,
metadataHandler,
/*artifactExpander=*/ null,
- /*remoteDefaultPlatformProperties=*/ ImmutableMap.of());
+ /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
+ /*isRemoteCacheEnabled=*/ true);
assertThat(token).isNull();
assertThat(output.getPath().exists()).isFalse();
@@ -763,7 +794,8 @@
/*handler=*/ null,
metadataHandler,
/*artifactExpander=*/ null,
- /*remoteDefaultPlatformProperties=*/ ImmutableMap.of());
+ /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
+ /*isRemoteCacheEnabled=*/ true);
TreeArtifactValue expectedMetadata = createTreeMetadata(output, children, Optional.empty());
assertThat(token).isNull();
@@ -890,7 +922,8 @@
/*handler=*/ null,
metadataHandler,
/*artifactExpander=*/ null,
- /*remoteDefaultPlatformProperties=*/ ImmutableMap.of());
+ /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
+ /*isRemoteCacheEnabled=*/ true);
assertThat(token).isNull();
assertStatistics(1, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build());
@@ -905,8 +938,9 @@
output,
ImmutableMap.of(
"file1",
- FileArtifactValue.createForTesting(output.getPath().getRelative("file1")),
- "file2", createRemoteFileMetadata("content2")),
+ FileArtifactValue.createForTesting(output.getPath().getRelative("file1")),
+ "file2",
+ createRemoteFileMetadata("content2")),
Optional.empty()));
}
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index 04dfaa3..286e508 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -3205,6 +3205,14 @@
}
function test_download_toplevel_when_turn_remote_cache_off() {
+ download_toplevel_when_turn_remote_cache_off
+}
+
+function test_download_toplevel_when_turn_remote_cache_off_with_metadata() {
+ download_toplevel_when_turn_remote_cache_off --experimental_action_cache_store_output_metadata
+}
+
+function download_toplevel_when_turn_remote_cache_off() {
# Test that BwtB doesn't cause build failure if remote cache is disabled in a following build.
# See https://github.com/bazelbuild/bazel/issues/13882.
@@ -3239,6 +3247,7 @@
bazel build \
--remote_cache=grpc://localhost:${worker_port} \
--remote_download_toplevel \
+ "$@" \
//a:consumer >& $TEST_log || fail "Failed to download outputs"
[[ -f bazel-bin/a/a.txt ]] || [[ -f bazel-bin/a/b.txt ]] \
&& fail "Expected outputs of producer are not downloaded"
@@ -3247,6 +3256,7 @@
echo 'bar' > a/in.txt
bazel build \
--remote_download_toplevel \
+ "$@" \
//a:consumer >& $TEST_log || fail "Failed to build without remote cache"
}
