Repository: bazelbuild/bazel
Issue: Bazel 0.29 contains a regression with Remote Build Execution backend
Labels: type: bug
P1
team-Remote-Exec

### Description of the problem / feature request:

Bazel 0.29.0 (https://github.com/bazelbuild/bazel/issues/8572) has regression with Remote Build Execution backend.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

ERROR: <redacted>/test/BUILD:205:5:  failed (Exit 34). Note: Remote connection/protocol failed with: execution failed FAILED_PRECONDITION: Precondition check failed.
 
### What operating system are you running Bazel on?

Linux 4.19.37-5+deb10u1rodete1-amd64 #1 SMP Debian 4.19.37-5+deb10u1rodete1 (2019-07-22 > 2018) x86_64 GNU/Linux

### What's the output of `bazel info release`?

release 0.29.0

Issue comments:
I am investigating with @v-mr 
We're seeing same issue in envoyproxy/envoy#8074, seems this only happens when `--nocache_test_results` is passed to bazel. I can reproduce it with `--nocache_test_results` but not without it.
Thank you for the data point, @lizan.

@v-mr has given me a repoduction case and I am bisecting now.
@lizan It's possible that the difference with `--nocache_test_results` is that when it is not set the tests aren't executed, which seems to be where the failure is.
> It's possible that the difference with --nocache_test_results is that when it is not set the tests aren't executed, which seems to be where the failure is.

@katre I doubt so because I changed the our RBE image SHA at same time, which should revoke all cache in the first time when I ran without `--nocache_test_results`.
Bisect has eventually told me that the culprit is e4ccba45376f74d24ce3506fb4057e1268f01a59. I have no idea why that is, but reverting it from master causes my reproduction to be fixed.

I'm going to debug this further to try and determine _why_ this change causes this failure.
That commit is the one that caused the action key to change, which is apparently what causes the bug to be visible. It's not the commit that actually causes the error.
Trying to repro. PRECONDITION_FAILED hints at inputs not being uploaded.
I believe the root cause is this line. We should unconditionally upload inputs: https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java;l=250?q=RemoteSpawnRunner

I ll send a fix. Sorry about this :(
Fix: https://github.com/bazelbuild/bazel/pull/9287
Here's a patch that applies cleanly on 0.29.0: https://github.com/buchgr/bazel/commit/c475d7d98a457e36a261f92ced9f9303e3f84c44
Came across this issue from a buildkite "Emergency" [message](https://buildkite.com/bazel/bazel-federation/builds/61). Since this issue is closed (and the message refers to a solution "later today"), can the message be removed?
@brandjon The core issue won't be fixed until 0.29.1 is released, hopefully tomorrow.

Pull request review comments:

Patch that resolved the issue:
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
index 56c2d8407f9448..8ff410f230c67d 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -245,17 +245,13 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
             () -> {
               ExecuteRequest request = requestBuilder.build();
 
-              // Upload the command and all the inputs into the remote cache, if remote caching
-              // is enabled and not disabled using tags, {@see Spawns#mayBeCachedRemotely}
-              if (spawnCacheableRemotely) {
-                try (SilentCloseable c = prof.profile(UPLOAD_TIME, "upload missing inputs")) {
-                  Map<Digest, Message> additionalInputs = Maps.newHashMapWithExpectedSize(2);
-                  additionalInputs.put(actionKey.getDigest(), action);
-                  additionalInputs.put(commandHash, command);
-                  remoteCache.ensureInputsPresent(merkleTree, additionalInputs, execRoot);
-                }
+              // Upload the command and all the inputs into the remote cache.
+              try (SilentCloseable c = prof.profile(UPLOAD_TIME, "upload missing inputs")) {
+                Map<Digest, Message> additionalInputs = Maps.newHashMapWithExpectedSize(2);
+                additionalInputs.put(actionKey.getDigest(), action);
+                additionalInputs.put(commandHash, command);
+                remoteCache.ensureInputsPresent(merkleTree, additionalInputs, execRoot);
               }
-
               ExecuteResponse reply;
               try (SilentCloseable c = prof.profile(REMOTE_EXECUTION, "execute remotely")) {
                 reply = remoteExecutor.executeRemotely(request);


Test patch:
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 6c787cbc488bb1..36d06126920f71 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
@@ -64,6 +64,7 @@
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
 import com.google.devtools.build.lib.exec.util.FakeOwner;
 import com.google.devtools.build.lib.remote.common.SimpleBlobStore.ActionKey;
+import com.google.devtools.build.lib.remote.merkletree.MerkleTree;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
@@ -212,7 +213,7 @@ public void nonCachableSpawnsShouldNotBeCached_localFallback() throws Exception
     runner.exec(spawn, policy);
 
     verify(localRunner).exec(spawn, policy);
-    verify(cache, never()).upload(any(), any(), any(), any(), any(), any());
+    verify(cache).ensureInputsPresent(any(), any(), eq(execRoot));
     verifyNoMoreInteractions(cache);
   }
 
diff --git a/src/test/shell/bazel/remote/remote_execution_http_test.sh b/src/test/shell/bazel/remote/remote_execution_http_test.sh
index a85df0cf6258f5..2925a1a12a55d2 100755
--- a/src/test/shell/bazel/remote/remote_execution_http_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_http_test.sh
@@ -426,4 +426,34 @@ EOF
   rm -rf $cache
 }
 
+function test_tag_no_remote_cache() {
+  mkdir -p a
+  cat > a/BUILD <<'EOF'
+genrule(
+  name = "foo",
+  srcs = [],
+  outs = ["foo.txt"],
+  cmd = "echo \"foo\" > \"$@\"",
+  tags = ["no-remote-cache"]
+)
+EOF
+
+  bazel build \
+    --spawn_strategy=local \
+    --remote_cache=grpc://localhost:${worker_port} \
+    //a:foo >& $TEST_log || "Failed to build //a:foo"
+
+  expect_log "1 local"
+
+  bazel clean
+
+  bazel build \
+    --spawn_strategy=local \
+    --remote_cache=grpc://localhost:${worker_port} \
+    //a:foo || "Failed to build //a:foo"
+
+  expect_log "1 local"
+  expect_not_log "remote cache hit"
+}
+
 run_suite "Remote execution and remote cache tests"
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index af6a303831e685..a296e9fe6d73a0 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -1195,6 +1195,56 @@ EOF
     //a:foo || "Failed to build //a:foo"
 }
 
+function test_tag_no_remote_cache() {
+  mkdir -p a
+  cat > a/BUILD <<'EOF'
+genrule(
+  name = "foo",
+  srcs = [],
+  outs = ["foo.txt"],
+  cmd = "echo \"foo\" > \"$@\"",
+  tags = ["no-remote-cache"]
+)
+EOF
+
+  bazel build \
+    --remote_executor=grpc://localhost:${worker_port} \
+    //a:foo >& $TEST_log || "Failed to build //a:foo"
+
+  expect_log "1 remote"
+
+  bazel clean
+
+  bazel build \
+    --remote_executor=grpc://localhost:${worker_port} \
+    //a:foo || "Failed to build //a:foo"
+
+  expect_log "1 remote"
+  expect_not_log "remote cache hit"
+}
+
+function test_tag_no_remote_exec() {
+  mkdir -p a
+  cat > a/BUILD <<'EOF'
+genrule(
+  name = "foo",
+  srcs = [],
+  outs = ["foo.txt"],
+  cmd = "echo \"foo\" > \"$@\"",
+  tags = ["no-remote-exec"]
+)
+EOF
+
+  bazel build \
+    --spawn_strategy=remote,local \
+    --remote_executor=grpc://localhost:${worker_port} \
+    //a:foo >& $TEST_log || "Failed to build //a:foo"
+
+  expect_log "1 local"
+  expect_not_log "1 remote"
+}
+
+
 # TODO(alpha): Add a test that fails remote execution when remote worker
 # supports sandbox.