Remote: Fix "file not found" error when remote cache is changed from enabled to disabled.

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,8 @@ 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/remote/options/RemoteOptions.java

@@ -549,10 +549,14 @@ public RemoteOutputsStrategyConverter() {
   /** The maximum size of an outbound message sent via a gRPC channel. */
   public int maxOutboundMessageSize = 1024 * 1024;
 
-  public boolean isRemoteEnabled() {
-    return !Strings.isNullOrEmpty(remoteCache) || isRemoteExecutionEnabled();
+  /** Returns {@code true} if remote cache or disk cache is enabled. */
+  public boolean isRemoteCacheEnabled() {
+    return !Strings.isNullOrEmpty(remoteCache)
+        || !(diskCache == null || diskCache.isEmpty())
+        || isRemoteExecutionEnabled();
   }
 
+  /** Returns {@code true} if remote execution is enabled. */
   public boolean isRemoteExecutionEnabled() {
     return !Strings.isNullOrEmpty(remoteExecutor);
   }

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/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -378,6 +378,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory, Configur
 
   private Map<String, String> lastRemoteDefaultExecProperties;
   private RemoteOutputsMode lastRemoteOutputsMode;
+  private Boolean lastRemoteCacheEnabled;
 
   class PathResolverFactoryImpl implements PathResolverFactory {
     @Override
@@ -1722,10 +1723,24 @@ private void deleteActionsIfRemoteOptionsChanged(OptionsProvider options)
         lastRemoteDefaultExecProperties != null
             && !remoteDefaultExecProperties.equals(lastRemoteDefaultExecProperties);
     lastRemoteDefaultExecProperties = remoteDefaultExecProperties;
+
+    boolean remoteCacheEnabled = remoteOptions != null && remoteOptions.isRemoteCacheEnabled();
+    // If we have remote metadata from last build, and the remote cache is not
+    // enabled in this build, invalidate actions since they can't download those
+    // remote files.
+    //
+    // TODO(chiwang): Re-evaluate this after action rewinding is implemented in
+    //  Bazel since we can treat that case as lost inputs.
+    if (lastRemoteOutputsMode != RemoteOutputsMode.ALL) {
+      needsDeletion |= lastRemoteCacheEnabled != null && lastRemoteCacheEnabled && !remoteCacheEnabled;
+    }
+    lastRemoteCacheEnabled = remoteCacheEnabled;
+
     RemoteOutputsMode remoteOutputsMode =
         remoteOptions != null ? remoteOptions.remoteOutputsMode : RemoteOutputsMode.ALL;
     needsDeletion |= lastRemoteOutputsMode != null && lastRemoteOutputsMode != remoteOutputsMode;
     this.lastRemoteOutputsMode = remoteOutputsMode;
+
     if (needsDeletion) {
       memoizingEvaluator.delete(k -> SkyFunctions.ACTION_EXECUTION.equals(k.functionName()));
     }

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=*/ false);
     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=*/ false))
         .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,32 @@ 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 +708,8 @@ public void saveOutputMetadata_emptyTreeMetadata_notSaved() throws Exception {
             /*handler=*/ null,
             metadataHandler,
             /*artifactExpander=*/ null,
-            /*remoteDefaultPlatformProperties=*/ ImmutableMap.of());
+            /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
+            /*isRemoteCacheEnabled=*/ false);
 
     assertThat(token).isNull();
     assertThat(output.getPath().exists()).isFalse();
@@ -763,7 +793,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 +921,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());

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

@@ -3204,4 +3204,101 @@ EOF
   expect_not_log "WARNING: Writing to Remote Cache:"
 }
 
+function test_download_toplevel_when_turn_remote_cache_off_without_metadata() {
+  # 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.
+
+  mkdir a
+  cat > a/BUILD <<'EOF'
+genrule(
+    name = "producer",
+    outs = ["a.txt", "b.txt"],
+    cmd = "touch $(OUTS)",
+)
+
+genrule(
+    name = "consumer",
+    outs = ["out.txt"],
+    srcs = [":b.txt", "in.txt"],
+    cmd = "cat $(SRCS) > $@",
+)
+EOF
+  echo 'foo' > a/in.txt
+
+  # populate the cache
+  bazel build \
+    --remote_cache=grpc://localhost:${worker_port} \
+    --remote_download_toplevel \
+    --verbose_failures \
+    //a:consumer >& $TEST_log || fail "Failed to populate the cache"
+
+  bazel clean >& $TEST_log || fail "Failed to clean"
+
+  # download top level outputs
+  bazel build \
+    --remote_cache=grpc://localhost:${worker_port} \
+    --remote_download_toplevel \
+    --verbose_failures \
+    //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"
+
+  # build without remote cache
+  echo 'bar' > a/in.txt
+  bazel build \
+    --remote_download_toplevel \
+    --verbose_failures \
+    //a:consumer >& $TEST_log || fail "Failed to build without remote cache"
+}
+
+function test_download_toplevel_when_turn_remote_cache_off_with_metadata() {
+  # Test that BwtB doesn't cause build failure if remote cache is disabled (but with
+  # --experimental_action_cache_store_output_metadata) in a following build.
+  # See https://github.com/bazelbuild/bazel/issues/13882.
+
+  mkdir a
+  cat > a/BUILD <<'EOF'
+genrule(
+    name = "producer",
+    outs = ["a.txt", "b.txt"],
+    cmd = "touch $(OUTS)",
+)
+
+genrule(
+    name = "consumer",
+    outs = ["out.txt"],
+    srcs = [":b.txt", "in.txt"],
+    cmd = "cat $(SRCS) > $@",
+)
+EOF
+  echo 'foo' > a/in.txt
+
+  # populate the cache
+  bazel build \
+    --remote_cache=grpc://localhost:${worker_port} \
+    --remote_download_toplevel \
+    --verbose_failures \
+    //a:consumer >& $TEST_log || fail "Failed to populate the cache"
+
+  bazel clean >& $TEST_log || fail "Failed to clean"
+
+  # download top level outputs
+  bazel build \
+    --remote_cache=grpc://localhost:${worker_port} \
+    --remote_download_toplevel \
+    --experimental_action_cache_store_output_metadata \
+    --verbose_failures \
+    //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"
+
+  # build without remote cache
+  echo 'bar' > a/in.txt
+  bazel build \
+    --remote_download_toplevel \
+    --experimental_action_cache_store_output_metadata \
+    --verbose_failures \
+    //a:consumer >& $TEST_log || fail "Failed to build without remote cache"
+}
+
 run_suite "Remote execution and remote cache tests"