Remote: Fixes an issue when --experimental_remote_cache_async encounter flaky tests.

coeuvre · copybara-github · commit f5cf8b076bc9 · 2021-11-07T20:07:24.000-08:00
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 bazelbuild#14008. Closes bazelbuild#14220. PiperOrigin-RevId: 408237437
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
@@ -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
@@ -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"
