Also handle remote cache eviction for tree artifacts. (#17601)

Related to #17366. PiperOrigin-RevId: 511849585 Change-Id: I1c4acd469280628b15441a8e7a97ff572c42c1ff Co-authored-by: Googler <tjgq@google.com>
bazelbuild · Feb 27, 2023 · 68e1924 · 68e1924
1 parent 10e6251
commit 68e1924
Show file tree

Hide file tree

Showing 7 changed files with 80 additions and 12 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
@@ -42,6 +42,7 @@
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.events.Reporter;
+import com.google.devtools.build.lib.remote.common.BulkTransferException;
 import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
 import com.google.devtools.build.lib.remote.util.AsyncTaskCache;
 import com.google.devtools.build.lib.remote.util.RxUtils.TransferResult;
@@ -252,7 +253,8 @@ private Completable prefetchInputTreeOrSymlink(
     PathFragment prefetchExecPath = treeMetadata.getMaterializationExecPath().orElse(execPath);
 
     Completable prefetch =
-        prefetchInputTree(context, provider, prefetchExecPath, treeFiles, treeMetadata, priority);
+        prefetchInputTree(
+            context, provider, prefetchExecPath, tree, treeFiles, treeMetadata, priority);
 
     // If prefetching to a different path, plant a symlink into it.
     if (!prefetchExecPath.equals(execPath)) {
@@ -288,6 +290,7 @@ private Completable prefetchInputTree(
       Context context,
       MetadataProvider provider,
       PathFragment execPath,
+      SpecialArtifact tree,
       List<TreeFileArtifact> treeFiles,
       FileArtifactValue treeMetadata,
       Priority priority) {
@@ -354,6 +357,12 @@ private Completable prefetchInputTree(
 
                   completed.set(true);
                 })
+            .doOnError(
+                error -> {
+                  if (BulkTransferException.anyCausedByCacheNotFoundException(error)) {
+                    missingActionInputs.add(tree);
+                  }
+                })
             .doFinally(
                 () -> {
                   if (!completed.get()) {

diff --git a/src/main/java/com/google/devtools/build/lib/remote/ExecuteRetrier.java b/src/main/java/com/google/devtools/build/lib/remote/ExecuteRetrier.java
@@ -89,7 +89,7 @@ public int getRetryAttempts() {
   }
 
   private static boolean shouldRetry(Exception e) {
-    if (BulkTransferException.isOnlyCausedByCacheNotFoundException(e)) {
+    if (BulkTransferException.allCausedByCacheNotFoundException(e)) {
       return true;
     }
     Status status = StatusProto.fromThrowable(e);

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java
@@ -104,7 +104,7 @@ protected ListenableFuture<Void> doDownloadFile(
   @Override
   protected Completable onErrorResumeNext(Throwable error) {
     if (error instanceof BulkTransferException) {
-      if (((BulkTransferException) error).onlyCausedByCacheNotFoundException()) {
+      if (((BulkTransferException) error).allCausedByCacheNotFoundException()) {
         var code =
             useNewExitCodeForLostInputs ? Code.REMOTE_CACHE_EVICTED : Code.REMOTE_CACHE_FAILED;
         error =

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -138,7 +138,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
       } catch (CacheNotFoundException e) {
         // Intentionally left blank
       } catch (IOException e) {
-        if (BulkTransferException.isOnlyCausedByCacheNotFoundException(e)) {
+        if (BulkTransferException.allCausedByCacheNotFoundException(e)) {
           // Intentionally left blank
         } else {
           String errorMessage;

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -225,7 +225,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
                 () -> action.getNetworkTime().getDuration(),
                 spawnMetrics);
           } catch (BulkTransferException e) {
-            if (!e.onlyCausedByCacheNotFoundException()) {
+            if (!e.allCausedByCacheNotFoundException()) {
               throw e;
             }
             // No cache hit, so we fall through to local or remote execution.
@@ -293,7 +293,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
                   () -> action.getNetworkTime().getDuration(),
                   spawnMetrics);
             } catch (BulkTransferException e) {
-              if (e.onlyCausedByCacheNotFoundException()) {
+              if (e.allCausedByCacheNotFoundException()) {
                 // No cache hit, so if we retry this execution, we must no longer accept
                 // cached results, it must be reexecuted
                 useCachedResult.set(false);
@@ -517,8 +517,7 @@ private SpawnResult execLocallyAndUploadOrFail(
   private SpawnResult handleError(
       RemoteAction action, IOException exception, SpawnExecutionContext context)
       throws ExecException, InterruptedException, IOException {
-    boolean remoteCacheFailed =
-        BulkTransferException.isOnlyCausedByCacheNotFoundException(exception);
+    boolean remoteCacheFailed = BulkTransferException.allCausedByCacheNotFoundException(exception);
     if (exception.getCause() instanceof ExecutionStatusException) {
       ExecutionStatusException e = (ExecutionStatusException) exception.getCause();
       if (e.getResponse() != null) {

diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/BulkTransferException.java b/src/main/java/com/google/devtools/build/lib/remote/common/BulkTransferException.java
@@ -23,8 +23,9 @@
  * with all constituent exceptions available for observation.
  */
 public class BulkTransferException extends IOException {
-  // true since no empty BulkTransferException is ever thrown
+  // No empty BulkTransferException is ever thrown.
   private boolean allCacheNotFoundException = true;
+  private boolean anyCacheNotFoundException = false;
 
   public BulkTransferException() {}
 
@@ -40,16 +41,26 @@ public BulkTransferException(IOException e) {
    */
   public void add(IOException e) {
     allCacheNotFoundException &= e instanceof CacheNotFoundException;
+    anyCacheNotFoundException |= e instanceof CacheNotFoundException;
     super.addSuppressed(e);
   }
 
-  public boolean onlyCausedByCacheNotFoundException() {
+  public boolean anyCausedByCacheNotFoundException() {
+    return anyCacheNotFoundException;
+  }
+
+  public static boolean anyCausedByCacheNotFoundException(Throwable e) {
+    return e instanceof BulkTransferException
+        && ((BulkTransferException) e).anyCausedByCacheNotFoundException();
+  }
+
+  public boolean allCausedByCacheNotFoundException() {
     return allCacheNotFoundException;
   }
 
-  public static boolean isOnlyCausedByCacheNotFoundException(Exception e) {
+  public static boolean allCausedByCacheNotFoundException(Throwable e) {
     return e instanceof BulkTransferException
-        && ((BulkTransferException) e).onlyCausedByCacheNotFoundException();
+        && ((BulkTransferException) e).allCausedByCacheNotFoundException();
   }
 
   @Override

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
@@ -513,4 +513,53 @@ public void remoteCacheEvictBlobs_incrementalBuildCanContinue() throws Exception
     // Assert: target was successfully built
     assertValidOutputFile("a/bar.out", "foo" + lineSeparator() + "updated bar" + lineSeparator());
   }
+
+  @Test
+  public void remoteCacheEvictBlobs_treeArtifact_incrementalBuildCanContinue() throws Exception {
+    // Arrange: Prepare workspace and populate remote cache
+    write("BUILD");
+    writeOutputDirRule();
+    write(
+        "a/BUILD",
+        "load('//:output_dir.bzl', 'output_dir')",
+        "output_dir(",
+        "  name = 'foo.out',",
+        "  manifest = ':manifest',",
+        ")",
+        "genrule(",
+        "  name = 'bar',",
+        "  srcs = ['foo.out', 'bar.in'],",
+        "  outs = ['bar.out'],",
+        "  cmd = '( ls $(location :foo.out); cat $(location :bar.in) ) > $@',",
+        "  tags = ['no-remote-exec'],",
+        ")");
+    write("a/manifest", "file-inside");
+    write("a/bar.in", "bar");
+
+    // Populate remote cache
+    buildTarget("//a:bar");
+    getOutputPath("a/foo.out").deleteTreesBelow();
+    getOutputPath("a/bar.out").delete();
+    getOutputBase().getRelative("action_cache").deleteTreesBelow();
+    restartServer();
+
+    // Clean build, foo.out isn't downloaded
+    buildTarget("//a:bar");
+    assertOutputDoesNotExist("a/foo.out/file-inside");
+
+    // Evict blobs from remote cache
+    worker.restart();
+
+    // trigger build error
+    write("a/bar.in", "updated bar");
+    // Build failed because of remote cache eviction
+    assertThrows(BuildFailedException.class, () -> buildTarget("//a:bar"));
+
+    // Act: Do an incremental build without "clean" or "shutdown"
+    buildTarget("//a:bar");
+
+    // Assert: target was successfully built
+    assertValidOutputFile(
+        "a/bar.out", "file-inside" + lineSeparator() + "updated bar" + lineSeparator());
+  }
 }