From f5cf8b076bc913dbe021104d5f6837fb4a6cd8b3 Mon Sep 17 00:00:00 2001
From: Chi Wang <chiwang@google.com>
Date: Sun, 7 Nov 2021 20:05:55 -0800
Subject: [PATCH] Remote: Fixes an issue when --experimental_remote_cache_async
 encounter flaky tests.

When `--experimental_remote_cache_async` is set, the uploads happened in the background -- usually after spawn execution.

When the test is failed and there is another test attempt, the outputs of previous test attempt are moved to other places immediately after spawn execution. This fine when combining `--experimental_remote_cache_async`  because outputs of failed action don't get uploaded.

However, there is an exception that `test.xml` is generated with another spawn before the "move" happens. The result of the spawn used to generate `test.xml` is usually "succeed" which means Bazel will attempt upload `test.xml` even if the test itself is failed.

This PR makes the `test.xml` generation spawn ignores remote cache if the test itself is failed.

Fixes #14008.

Closes #14220.

PiperOrigin-RevId: 408237437
---
 .../lib/exec/StandaloneTestStrategy.java      | 12 ++++++-
 .../bazel/remote/remote_execution_test.sh     | 33 +++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
index 1bedef5c5936a6..569e419566e1ea 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -20,6 +20,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Maps;
 import com.google.common.io.ByteStreams;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
@@ -535,13 +536,22 @@ private static Spawn createXmlGeneratingSpawn(
       envBuilder.put("TEST_SHARD_INDEX", "0");
       envBuilder.put("TEST_TOTAL_SHARDS", "0");
     }
+    Map<String, String> executionInfo =
+        Maps.newHashMapWithExpectedSize(action.getExecutionInfo().size() + 1);
+    executionInfo.putAll(action.getExecutionInfo());
+    if (result.exitCode() != 0) {
+      // If the test is failed, the spawn shouldn't use remote cache since the test.xml file is
+      // renamed immediately after the spawn execution. If there is another test attempt, the async
+      // upload will fail because it cannot read the file at original position.
+      executionInfo.put(ExecutionRequirements.NO_REMOTE_CACHE, "");
+    }
     return new SimpleSpawn(
         action,
         args,
         envBuilder.build(),
         // Pass the execution info of the action which is identical to the supported tags set on the
         // test target. In particular, this does not set the test timeout on the spawn.
-        ImmutableMap.copyOf(action.getExecutionInfo()),
+        ImmutableMap.copyOf(executionInfo),
         null,
         ImmutableMap.of(),
         /*inputs=*/ NestedSetBuilder.create(
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index 3c5735e39e3f0a..3a3218cb79f66f 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -3171,4 +3171,37 @@ EOF
   expect_log "-r-xr-xr-x"
 }
 
+function test_async_upload_works_for_flaky_tests() {
+  mkdir -p a
+  cat > a/BUILD <<EOF
+sh_test(
+    name = "test",
+    srcs = ["test.sh"],
+)
+
+genrule(
+  name = "foo",
+  outs = ["foo.txt"],
+  cmd = "echo \"foo bar\" > \$@",
+)
+EOF
+  cat > a/test.sh <<EOF
+#!/bin/sh
+echo "it always fails"
+exit 1
+EOF
+  chmod +x a/test.sh
+
+  # Check the error message when failed to upload
+  bazel build --remote_cache=http://nonexistent.example.org //a:foo >& $TEST_log || fail "Failed to build"
+  expect_log "WARNING: Writing to Remote Cache:"
+
+  bazel test \
+    --remote_cache=grpc://localhost:${worker_port} \
+    --experimental_remote_cache_async \
+    --flaky_test_attempts=2 \
+    //a:test >& $TEST_log  && fail "expected failure" || true
+  expect_not_log "WARNING: Writing to Remote Cache:"
+}
+
 run_suite "Remote execution and remote cache tests"