Retry ensureInputsPresent/execute/download

This observably removes any ill effect of CAS transience.

Closes #5229.

PiperOrigin-RevId: 204010317

3 files changed, 43 insertions, 38 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
index 5c45d45944..849259f4a2 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
@@ -277,6 +277,7 @@ public class GrpcRemoteExecutionClientTest {
             "command-id",
             remoteCache,
             executor,
+            retrier,
             DIGEST_UTIL,
             logDir);
     inputDigest = fakeFileCache.createScratchInput(simpleSpawn.getInputFiles().get(0), "xyz");
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
index 283cc956bd..6b024b58aa 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -30,6 +30,8 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.eventbus.EventBus;
 import com.google.common.io.ByteStreams;
+import com.google.common.util.concurrent.ListeningScheduledExecutorService;
+import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.SettableFuture;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
@@ -79,7 +81,10 @@ import java.nio.charset.StandardCharsets;
 import java.time.Duration;
 import java.util.Collection;
 import java.util.SortedMap;
+import java.util.concurrent.Executors;
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -94,6 +99,7 @@ public class RemoteSpawnRunnerTest {
 
   private static final ImmutableMap<String, String> NO_CACHE =
       ImmutableMap.of(ExecutionRequirements.NO_CACHE, "");
+  private static ListeningScheduledExecutorService retryService;
 
   private Path execRoot;
   private Path logDir;
@@ -101,6 +107,9 @@ public class RemoteSpawnRunnerTest {
   private FakeActionInputFileCache fakeFileCache;
   private FileOutErr outErr;
 
+  private RemoteOptions options;
+  private RemoteRetrier retrier;
+
   @Mock private AbstractRemoteActionCache cache;
 
   @Mock
@@ -113,6 +122,11 @@ public class RemoteSpawnRunnerTest {
   private final String simpleActionId =
       "eb45b20cc979d504f96b9efc9a08c48103c6f017afa09c0df5c70a5f92a98ea8";
 
+  @BeforeClass
+  public static void beforeEverything() {
+    retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1));
+  }
+
   @Before
   public final void setUp() throws Exception {
     MockitoAnnotations.initMocks(this);
@@ -128,6 +142,14 @@ public class RemoteSpawnRunnerTest {
     FileSystemUtils.createDirectoryAndParents(stdout.getParentDirectory());
     FileSystemUtils.createDirectoryAndParents(stderr.getParentDirectory());
     outErr = new FileOutErr(stdout, stderr);
+
+    options = Options.getDefaults(RemoteOptions.class);
+    retrier = RemoteModule.createExecuteRetrier(options, retryService);
+  }
+
+  @AfterClass
+  public static void afterEverything() {
+    retryService.shutdownNow();
   }
 
   @Test
@@ -137,7 +159,6 @@ public class RemoteSpawnRunnerTest {
     // It should be executed remotely, but marked non-cacheable to remote execution, so that
     // the action result is not saved in the remote cache.
 
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteAcceptCached = true;
     options.remoteLocalFallback = false;
     options.remoteUploadLocalResults = true;
@@ -154,6 +175,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -197,7 +219,6 @@ public class RemoteSpawnRunnerTest {
     // Test that if a spawn is executed locally, due to the local fallback, that its result is not
     // uploaded to the remote cache. However, the artifacts should still be uploaded.
 
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteAcceptCached = true;
     options.remoteLocalFallback = true;
     options.remoteUploadLocalResults = true;
@@ -214,6 +235,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             null,
+            retrier,
             digestUtil,
             logDir);
 
@@ -252,7 +274,6 @@ public class RemoteSpawnRunnerTest {
     // Test that the outputs of a failed locally executed action are uploaded to a remote cache,
     // but the action result itself is not.
 
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteUploadLocalResults = true;
 
     RemoteSpawnRunner runner =
@@ -268,6 +289,7 @@ public class RemoteSpawnRunnerTest {
                 "command-id",
                 cache,
                 null,
+                retrier,
                 digestUtil,
                 logDir));
 
@@ -318,6 +340,7 @@ public class RemoteSpawnRunnerTest {
                 "command-id",
                 cache,
                 null,
+                retrier,
                 digestUtil,
                 logDir));
 
@@ -334,7 +357,6 @@ public class RemoteSpawnRunnerTest {
   public void printWarningIfCacheIsDown() throws Exception {
     // If we try to upload to a local cache, that is down a warning should be printed.
 
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteUploadLocalResults = true;
     options.remoteLocalFallback = true;
 
@@ -354,6 +376,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             null,
+            retrier,
             digestUtil,
             logDir);
 
@@ -396,7 +419,6 @@ public class RemoteSpawnRunnerTest {
   public void noRemoteExecutorFallbackFails() throws Exception {
     // Errors from the fallback runner should be propogated out of the remote runner.
 
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteUploadLocalResults = true;
     options.remoteLocalFallback = true;
 
@@ -412,6 +434,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             null,
+            retrier,
             digestUtil,
             logDir);
 
@@ -437,7 +460,6 @@ public class RemoteSpawnRunnerTest {
   public void remoteCacheErrorFallbackFails() throws Exception {
     // Errors from the fallback runner should be propogated out of the remote runner.
 
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteUploadLocalResults = true;
     options.remoteLocalFallback = true;
 
@@ -453,6 +475,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             null,
+            retrier,
             digestUtil,
             logDir);
 
@@ -476,7 +499,6 @@ public class RemoteSpawnRunnerTest {
 
   @Test
   public void testLocalFallbackFailureRemoteExecutorFailure() throws Exception {
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteLocalFallback = true;
 
     RemoteSpawnRunner runner =
@@ -491,6 +513,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -527,6 +550,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -568,6 +592,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -612,6 +637,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -651,6 +677,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -680,8 +707,6 @@ public class RemoteSpawnRunnerTest {
   public void cacheDownloadFailureTriggersRemoteExecution() throws Exception {
     // If downloading a cached action fails, remote execution should be tried.
 
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
-
     RemoteSpawnRunner runner =
         new RemoteSpawnRunner(
             execRoot,
@@ -694,6 +719,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -722,7 +748,6 @@ public class RemoteSpawnRunnerTest {
   public void testRemoteExecutionTimeout() throws Exception {
     // If remote execution times out the SpawnResult status should be TIMEOUT.
 
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteLocalFallback = false;
 
     RemoteSpawnRunner runner =
@@ -737,6 +762,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -771,7 +797,6 @@ public class RemoteSpawnRunnerTest {
     // If remote execution times out the SpawnResult status should be TIMEOUT, regardess of local
     // fallback option.
 
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteLocalFallback = true;
 
     RemoteSpawnRunner runner =
@@ -786,6 +811,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -818,7 +844,6 @@ public class RemoteSpawnRunnerTest {
 
   @Test
   public void testRemoteExecutionCommandFailureDoesNotTriggerFallback() throws Exception {
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteLocalFallback = true;
 
     RemoteSpawnRunner runner =
@@ -833,6 +858,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -860,7 +886,6 @@ public class RemoteSpawnRunnerTest {
     // If we get a failure due to the remote cache not working, the exit code should be
     // ExitCode.REMOTE_ERROR.
 
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteLocalFallback = false;
 
     RemoteSpawnRunner runner =
@@ -875,6 +900,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -898,7 +924,6 @@ public class RemoteSpawnRunnerTest {
     // If we get a failure due to the remote executor not working, the exit code should be
     // ExitCode.REMOTE_ERROR.
 
-    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.remoteLocalFallback = false;
 
     RemoteSpawnRunner runner =
@@ -913,6 +938,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
@@ -947,6 +973,7 @@ public class RemoteSpawnRunnerTest {
             "command-id",
             cache,
             executor,
+            retrier,
             digestUtil,
             logDir);
 
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java b/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java
index 624d074468..7dde0c6051 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.remote.Retrier.CircuitBreaker;
 import com.google.devtools.build.lib.remote.Retrier.CircuitBreaker.State;
 import com.google.devtools.build.lib.remote.Retrier.CircuitBreakerException;
 import com.google.devtools.build.lib.remote.Retrier.RetryException;
+import com.google.devtools.build.lib.remote.Retrier.ZeroBackoff;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executors;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -318,28 +319,4 @@ public class RetrierTest {
       state = State.TRIAL_CALL;
     }
   }
-
-  private static class ZeroBackoff implements Backoff {
-
-    private final int maxRetries;
-    private int retries;
-
-    public ZeroBackoff(int maxRetries) {
-      this.maxRetries = maxRetries;
-    }
-
-    @Override
-    public long nextDelayMillis() {
-      if (retries >= maxRetries) {
-        return -1;
-      }
-      retries++;
-      return 0;
-    }
-
-    @Override
-    public int getRetryAttempts() {
-      return retries;
-    }
-  }
 }