From 55392987085e3124f908e8f6242b9688d7dff378 Mon Sep 17 00:00:00 2001
From: Googler <chiwang@google.com>
Date: Wed, 5 Jul 2023 05:37:03 -0700
Subject: [PATCH] Do not invalidate skyframe's in-memory action cache when
 changing output mode.

Previously, we invalidate skyframe's cache if the output mode is changed from BwoB to ALL because they were two different code paths and are incompatible with each other.

Now the code paths are unifded and changing the output mode only affects the decision for whether Bazel should download outputs, no more other things.

PiperOrigin-RevId: 545641839
Change-Id: I83288f7cb505ad0e47368c702797ca365bdb4191
---
 .../build/lib/skyframe/SkyframeExecutor.java    |  5 +----
 .../BuildWithoutTheBytesIntegrationTest.java    | 17 +++++++++++------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 2e4ba6b327bf56..5a74f4933f205b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1722,11 +1722,8 @@ public void deleteActionsIfRemoteOptionsChanged(OptionsProvider options)
           lastRemoteCacheEnabled != null && lastRemoteCacheEnabled && !remoteCacheEnabled;
     }
     lastRemoteCacheEnabled = remoteCacheEnabled;
-
-    RemoteOutputsMode remoteOutputsMode =
+    lastRemoteOutputsMode =
         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()));
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
index 412bdd0bc90e21..b568707e64f280 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
@@ -194,7 +194,7 @@ public void intermediateOutputsAreInputForInternalActions_prefetchIntermediateOu
   }
 
   @Test
-  public void changeOutputMode_invalidateActions() throws Exception {
+  public void changeOutputMode_notInvalidateActions() throws Exception {
     write(
         "a/BUILD",
         "genrule(",
@@ -210,21 +210,26 @@ public void changeOutputMode_invalidateActions() throws Exception {
         "  outs = ['foobar.txt'],",
         "  cmd = 'cat $(location :foo) > $@ && echo bar > $@',",
         ")");
+    // Download all outputs with regex so in the next build with ALL mode, the actions are not
+    // invalidated because of missing outputs.
+    addOptions("--experimental_remote_download_regex=.*");
     ActionEventCollector actionEventCollector = new ActionEventCollector();
     runtimeWrapper.registerSubscriber(actionEventCollector);
     buildTarget("//a:foobar");
+    // Add the new option here because waitDownloads below will internally create a new command
+    // which will parse the new option.
+    setDownloadAll();
+    waitDownloads();
     // 3 = workspace status action + //:foo + //:foobar
     assertThat(actionEventCollector.getNumActionNodesEvaluated()).isEqualTo(3);
     actionEventCollector.clear();
     events.clear();
 
-    setDownloadAll();
     buildTarget("//a:foobar");
 
-    // Changing output mode should invalidate SkyFrame's in-memory caching and make it re-evaluate
-    // the action nodes.
-    assertThat(actionEventCollector.getNumActionNodesEvaluated()).isEqualTo(3);
-    events.assertContainsInfo("2 processes: 2 remote cache hit");
+    // Changing output mode should not invalidate SkyFrame's in-memory caching.
+    assertThat(actionEventCollector.getNumActionNodesEvaluated()).isEqualTo(0);
+    events.assertContainsInfo("0 processes");
   }
 
   @Test