Remote: Don't load remote metadata from action cache if remote cache …

…is disabled.

Part of bazelbuild#14252.

Closes bazelbuild#14252.

PiperOrigin-RevId: 410448656

src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java

@@ -414,7 +414,8 @@ public Token getTokenIfNeedToExecute(
       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 @@ public Token getTokenIfNeedToExecute(
     }
     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);
     }
 

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java

@@ -599,6 +599,7 @@ Token checkActionCache(
           remoteOptions != null
               ? remoteOptions.getRemoteDefaultExecProperties()
               : ImmutableSortedMap.of();
+      boolean isRemoteCacheEnabled = remoteOptions != null && remoteOptions.isRemoteCacheEnabled();
       token =
           actionCacheChecker.getTokenIfNeedToExecute(
               action,
@@ -609,7 +610,8 @@ Token checkActionCache(
                   : null,
               metadataHandler,
               artifactExpander,
-              remoteDefaultProperties);
+              remoteDefaultProperties,
+              isRemoteCacheEnabled);
     } catch (UserExecException e) {
       throw e.toActionExecutionException(action);
     }

src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java

@@ -183,7 +183,8 @@ private void runAction(
             /*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 @@ public void testDeletedConstantMetadataOutputCausesReexecution() throws Exceptio
                 /*handler=*/ null,
                 new FakeMetadataHandler(),
                 /*artifactExpander=*/ null,
-                /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()))
+                /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
+                /*isRemoteCacheEnabled=*/ true))
         .isNotNull();
   }
 
@@ -557,7 +559,8 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception {
             /*handler=*/ null,
             metadataHandler,
             /*artifactExpander=*/ null,
-            /*remoteDefaultPlatformProperties=*/ ImmutableMap.of());
+            /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
+            /*isRemoteCacheEnabled=*/ true);
 
     assertThat(output.getPath().exists()).isFalse();
     assertThat(token).isNull();
@@ -567,6 +570,33 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception {
     assertThat(metadataHandler.getMetadata(output)).isEqualTo(createRemoteFileMetadata(content));
   }
 
+  @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);
@@ -679,7 +709,8 @@ public void saveOutputMetadata_emptyTreeMetadata_notSaved() throws Exception {
             /*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 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Ex
             /*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 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th
             /*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 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th
                 output,
                 ImmutableMap.of(
                     "file1",
-                        FileArtifactValue.createForTesting(output.getPath().getRelative("file1")),
-                    "file2", createRemoteFileMetadata("content2")),
+                    FileArtifactValue.createForTesting(output.getPath().getRelative("file1")),
+                    "file2",
+                    createRemoteFileMetadata("content2")),
                 Optional.empty()));
   }
 

src/test/shell/bazel/remote/remote_execution_test.sh

@@ -3205,6 +3205,14 @@ EOF
 }
 
 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 @@ EOF
   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 @@ EOF
   echo 'bar' > a/in.txt
   bazel build \
     --remote_download_toplevel \
+    "$@" \
     //a:consumer >& $TEST_log || fail "Failed to build without remote cache"
 }