Take the no-remote-exec tag into account when computing the action salt

Multiple times have I noticed build breakages, where people add/remove the "no-remote-exec" annotation from targets, which only came to light after existing remote cache entries expire.

For example, people may try to promote a test to run remotely. "bazel test" shows the test passes, so they assume the test is safe to run remotely. Later on, the cache entry for that test expires. Or the test changes, causing its action cache key to be different. The test gets rebuilt remotely. This now fails, because the test never succeeded remotely to begin with. The cache entry belonged to the action that ran locally.

Let's formalize the schema of the "salt" field. Instead of reusing the Platform message, let's declare our own message that we can use to add arbitrary properties that should be taken into account when determining the action's uniqueness.

Closes #17304.

PiperOrigin-RevId: 505010966
Change-Id: Ib88e769795f18b1724895ebd79835b2bc3b13e1e

Author: EdSchouten

src/main/java/com/google/devtools/build/lib/remote/BUILD

@@ -112,6 +112,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/skyframe",
         "//src/main/java/com/google/devtools/common/options",
         "//src/main/protobuf:failure_details_java_proto",
+        "//src/main/protobuf:spawn_java_proto",
         "//third_party:auth",
         "//third_party:caffeine",
         "//third_party:flogger",

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

@@ -76,6 +76,7 @@
 import com.google.devtools.build.lib.buildtool.buildevent.BuildInterruptedEvent;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Reporter;
+import com.google.devtools.build.lib.exec.Protos.CacheSalt;
 import com.google.devtools.build.lib.exec.SpawnInputExpander.InputWalker;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
 import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
@@ -425,18 +426,16 @@ public MerkleTree uncachedBuildMerkleTreeVisitor(
 
   @Nullable
   private static ByteString buildSalt(Spawn spawn) {
+    CacheSalt.Builder saltBuilder =
+        CacheSalt.newBuilder().setMayBeExecutedRemotely(Spawns.mayBeExecutedRemotely(spawn));
+
     String workspace =
         spawn.getExecutionInfo().get(ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE);
     if (workspace != null) {
-      Platform platform =
-          Platform.newBuilder()
-              .addProperties(
-                  Platform.Property.newBuilder().setName("workspace").setValue(workspace).build())
-              .build();
-      return platform.toByteString();
+      saltBuilder.setWorkspace(workspace);
     }
 
-    return null;
+    return saltBuilder.build().toByteString();
   }
 
   /**

src/main/protobuf/spawn.proto

@@ -185,3 +185,17 @@ message SpawnExec {
   // Timing, size and memory statistics.
   SpawnMetrics metrics = 20;
 }
+
+// Additional information that should be taken into account when
+// computing the key of an action, thereby ensuring that actions remain
+// distinct.
+message CacheSalt {
+  // Whether or not the action may be executed remotely, if remote
+  // execution were to be enabled. This ensures that adding/removing the
+  // "no-remote-exec" tag from a target forces a local/remote rebuild.
+  bool may_be_executed_remotely = 1;
+
+  // Requires the execution service do NOT share caches across different
+  // workspace.
+  string workspace = 2;
+}

src/test/java/com/google/devtools/build/lib/remote/BUILD

@@ -93,6 +93,7 @@ java_test(
         "//src/main/java/com/google/devtools/common/options",
         "//src/main/protobuf:failure_details_java_proto",
         "//src/main/protobuf:remote_execution_log_java_proto",
+        "//src/main/protobuf:spawn_java_proto",
         "//src/test/java/com/google/devtools/build/lib:test_runner",
         "//src/test/java/com/google/devtools/build/lib/actions/util",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",

src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java

@@ -79,6 +79,7 @@
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.events.StoredEventHandler;
+import com.google.devtools.build.lib.exec.Protos.CacheSalt;
 import com.google.devtools.build.lib.exec.util.FakeOwner;
 import com.google.devtools.build.lib.exec.util.SpawnBuilder;
 import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult;
@@ -258,7 +259,7 @@ public void buildRemoteAction_withActionInputDirectoryAsOutput() throws Exceptio
   }
 
   @Test
-  public void buildRemoteAction_differentiateWorkspace_generateActionSalt() throws Exception {
+  public void buildRemoteAction_generateActionSalt_differentiateWorkspaceCache() throws Exception {
     Spawn spawn =
         new SpawnBuilder("dummy")
             .withExecutionInfo(ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE, "aa")
@@ -268,10 +269,23 @@ public void buildRemoteAction_differentiateWorkspace_generateActionSalt() throws
 
     RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
 
-    Platform expected =
-        Platform.newBuilder()
-            .addProperties(Platform.Property.newBuilder().setName("workspace").setValue("aa"))
+    CacheSalt expected =
+        CacheSalt.newBuilder().setMayBeExecutedRemotely(true).setWorkspace("aa").build();
+    assertThat(remoteAction.getAction().getSalt()).isEqualTo(expected.toByteString());
+  }
+
+  @Test
+  public void buildRemoteAction_generateActionSalt_noRemoteExec() throws Exception {
+    Spawn spawn =
+        new SpawnBuilder("dummy")
+            .withExecutionInfo(ExecutionRequirements.NO_REMOTE_EXEC, "")
             .build();
+    FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+    RemoteExecutionService service = newRemoteExecutionService();
+
+    RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
+
+    CacheSalt expected = CacheSalt.newBuilder().setMayBeExecutedRemotely(false).build();
     assertThat(remoteAction.getAction().getSalt()).isEqualTo(expected.toByteString());
   }
 

src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java

@@ -152,7 +152,7 @@ public class RemoteSpawnRunnerTest {
 
   // The action key of the Spawn returned by newSimpleSpawn().
   private final String simpleActionId =
-      "eb45b20cc979d504f96b9efc9a08c48103c6f017afa09c0df5c70a5f92a98ea8";
+      "31aea267dc597b047a9b6993100415b6406f82822318dc8988e4164a535b51ee";
 
   @Before
   public final void setUp() throws Exception {
@@ -557,7 +557,7 @@ public void testHumanReadableServerLogsSavedForFailingActionWithSiblingRepositor
     Digest logDigest = digestUtil.computeAsUtf8("bla");
     Path logPath =
         logDir
-            .getRelative("b9a727771337fd8ce54821f4805e2d451c4739e92fec6f8ecdb18ff9d1983b27")
+            .getRelative("e0a5a3561464123504c1240b3587779cdfd6adee20f72aa136e388ecfd570c12")
             .getRelative("logname");
     ExecuteResponse resp =
         ExecuteResponse.newBuilder()