From 0a2aac4e3c6f5ef98fc233e2ce83e44930edb188 Mon Sep 17 00:00:00 2001
From: Googler <jhorvitz@google.com>
Date: Tue, 17 Oct 2023 08:25:23 -0700
Subject: [PATCH] Make action rewinding compatible with `--use_action_cache`.

When preparing an action for rewinding, evict it from the action cache to ensure that it is executed.

PiperOrigin-RevId: 574158458
Change-Id: I82075f9a1a79cdd601ac6e0d2b3c17969dae81ca
---
 .../devtools/build/lib/actions/ActionCacheChecker.java |  3 ++-
 .../build/lib/skyframe/SequencedSkyframeExecutor.java  | 10 ----------
 .../build/lib/skyframe/SkyframeActionExecutor.java     |  4 ++++
 .../lib/skyframe/SequencedSkyframeExecutorTest.java    |  7 ++-----
 .../build/lib/skyframe/rewinding/RewindingTest.java    |  1 -
 5 files changed, 8 insertions(+), 17 deletions(-)

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 56f5ed0712a51d..adc8992a872447 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
@@ -149,7 +149,8 @@ private ActionCache.Entry getCacheEntry(Action action) {
     return ActionCacheUtils.getCacheEntry(actionCache, action);
   }
 
-  private void removeCacheEntry(Action action) {
+  public void removeCacheEntry(Action action) {
+    checkState(enabled(), "Action cache disabled");
     ActionCacheUtils.removeCacheEntry(actionCache, action);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index 1efb78cdd1fa1c..a9c9215768bc65 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -317,16 +317,6 @@ private boolean rewindingEnabled(OptionsProvider options) throws AbruptExitExcep
     if (buildRequestOptions == null || !buildRequestOptions.rewindLostInputs) {
       return false;
     }
-    if (buildRequestOptions.useActionCache) {
-      throw new AbruptExitException(
-          DetailedExitCode.of(
-              FailureDetail.newBuilder()
-                  .setMessage("--rewind_lost_inputs requires --nouse_action_cache")
-                  .setActionRewinding(
-                      ActionRewinding.newBuilder()
-                          .setCode(ActionRewinding.Code.REWIND_LOST_INPUTS_PREREQ_UNMET))
-                  .build()));
-    }
     if (isMergedSkyframeAnalysisExecution()) {
       throw new AbruptExitException(
           DetailedExitCode.of(
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 4a830963fec9b3..b75b2cebdde2fb 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
@@ -464,6 +464,10 @@ private void prepareDepForRewinding(ActionLookupData failedKey, Action dep) {
     if (!actionFileSystemType().inMemoryFileSystem()) {
       outputDirectoryHelper.invalidateTreeArtifactDirectoryCreation(dep.getOutputs());
     }
+    // Evict the rewinding action from the action cache to ensure that it is executed.
+    if (actionCacheChecker.enabled()) {
+      actionCacheChecker.removeCacheEntry(dep);
+    }
   }
 
   @Nullable
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
index 75a46130ffc7ac..bfde8fec39b5d6 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
@@ -2664,14 +2664,11 @@ public NestedSet<Artifact> discoverInputs(ActionExecutionContext actionExecution
 
   @Test
   public void rewindingPrerequisites(
-      @TestParameter boolean trackIncrementalState,
-      @TestParameter boolean useActionCache,
-      @TestParameter boolean skymeldEnabled)
+      @TestParameter boolean trackIncrementalState, @TestParameter boolean skymeldEnabled)
       throws Exception {
     initializeSkyframeExecutor();
     options.parse(
         "--rewind_lost_inputs",
-        "--use_action_cache=" + useActionCache,
         "--experimental_merged_skyframe_analysis_execution=" + skymeldEnabled);
     skyframeExecutor.setMergedSkyframeAnalysisExecutionSupplier(() -> skymeldEnabled);
 
@@ -2685,7 +2682,7 @@ public void rewindingPrerequisites(
         reporter);
     skyframeExecutor.setActive(true);
 
-    if (useActionCache || skymeldEnabled) {
+    if (skymeldEnabled) {
       AbruptExitException e = assertThrows(AbruptExitException.class, this::syncSkyframeExecutor);
       assertThat(e.getDetailedExitCode().getFailureDetail().getActionRewinding().getCode())
           .isEqualTo(ActionRewinding.Code.REWIND_LOST_INPUTS_PREREQ_UNMET);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTest.java
index 522b8e8ddc32cf..44183d32438e4a 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTest.java
@@ -71,7 +71,6 @@ protected void setupOptions() throws Exception {
     super.setupOptions();
     addOptions(
         "--spawn_strategy=standalone",
-        "--nouse_action_cache",
         "--noexperimental_merged_skyframe_analysis_execution",
         "--rewind_lost_inputs",
         "--features=cc_include_scanning",