Remote: Fixed a bug that remote cache is missed due to executable bit is changed

Fixes #13262.

#12820 changed to set executable bit of input files based on its real value. However, this causes cache misses in `--remote_download_toplevel` mode since executable bit is changed after action execution by `ActionMetadataHandler#getMetadata`.

This change effectively rolls back #12820.

Closes #13276.

PiperOrigin-RevId: 367009617

Author: coeuvre

src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java

@@ -74,21 +74,37 @@ static class FileNode extends Node {
     private final Digest digest;
     private final boolean isExecutable;
 
-    FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable) {
+    /**
+     * Create a FileNode with its executable bit set.
+     *
+     * <p>We always treat files as executable since Bazel will `chmod 555` on the output files of an
+     * action within ActionMetadataHandler#getMetadata after action execution if no metadata was
+     * injected. We can't use real executable bit of the file until this behaviour is changed. See
+     * https://github.com/bazelbuild/bazel/issues/13262 for more details.
+     */
+    static FileNode createExecutable(String pathSegment, Path path, Digest digest) {
+      return new FileNode(pathSegment, path, digest, /* isExecutable= */ true);
+    }
+
+    static FileNode createExecutable(String pathSegment, ByteString data, Digest digest) {
+      return new FileNode(pathSegment, data, digest, /* isExecutable= */ true);
+    }
+
+    private FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable) {
       super(pathSegment);
       this.path = Preconditions.checkNotNull(path, "path");
       this.data = null;
       this.digest = Preconditions.checkNotNull(digest, "digest");
       this.isExecutable = isExecutable;
     }
 
-    FileNode(String pathSegment, ByteString data, Digest digest) {
+    private FileNode(String pathSegment, ByteString data, Digest digest, boolean isExecutable) {
       super(pathSegment);
       this.path = null;
       this.data = Preconditions.checkNotNull(data, "data");
       ;
       this.digest = Preconditions.checkNotNull(digest, "digest");
-      this.isExecutable = false;
+      this.isExecutable = isExecutable;
     }
 
     Digest getDigest() {

src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java

@@ -20,7 +20,6 @@
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.MetadataProvider;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
-import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue;
 import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.DirectoryNode;
 import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.FileNode;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
@@ -102,7 +101,7 @@ private static int buildFromPaths(
             throw new IOException(String.format("Input '%s' is not a file.", input));
           }
           Digest d = digestUtil.compute(input);
-          currDir.addChild(new FileNode(path.getBaseName(), input, d, input.isExecutable()));
+          currDir.addChild(FileNode.createExecutable(path.getBaseName(), input, d));
           return 1;
         });
   }
@@ -128,7 +127,8 @@ private static int buildFromActionInputs(
           if (input instanceof VirtualActionInput) {
             VirtualActionInput virtualActionInput = (VirtualActionInput) input;
             Digest d = digestUtil.compute(virtualActionInput);
-            currDir.addChild(new FileNode(path.getBaseName(), virtualActionInput.getBytes(), d));
+            currDir.addChild(
+                FileNode.createExecutable(path.getBaseName(), virtualActionInput.getBytes(), d));
             return 1;
           }
 
@@ -141,15 +141,7 @@ private static int buildFromActionInputs(
             case REGULAR_FILE:
               Digest d = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());
               Path inputPath = ActionInputHelper.toInputPath(input, execRoot);
-
-              boolean isExecutable;
-              if (metadata instanceof RemoteActionFileArtifactValue) {
-                isExecutable = ((RemoteActionFileArtifactValue) metadata).isExecutable();
-              } else {
-                isExecutable = inputPath.isExecutable();
-              }
-
-              currDir.addChild(new FileNode(path.getBaseName(), inputPath, d, isExecutable));
+              currDir.addChild(FileNode.createExecutable(path.getBaseName(), inputPath, d));
               return 1;
 
             case DIRECTORY:

src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java

@@ -70,9 +70,9 @@ public void virtualActionInputShouldWork() throws Exception {
     assertThat(directoriesAtDepth(1, tree)).isEmpty();
 
     FileNode expectedFooNode =
-        new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false);
+        FileNode.createExecutable("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"));
     FileNode expectedBarNode =
-        new FileNode("bar.cc", bar.getBytes(), digestUtil.computeAsUtf8("bar"));
+        FileNode.createExecutable("bar.cc", bar.getBytes(), digestUtil.computeAsUtf8("bar"));
     assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
     assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode, expectedBarNode);
   }
@@ -117,19 +117,13 @@ public void directoryInputShouldBeExpanded() throws Exception {
     assertThat(directoriesAtDepth(3, tree)).isEmpty();
 
     FileNode expectedFooNode =
-        new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false);
+        FileNode.createExecutable("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"));
     FileNode expectedBarNode =
-        new FileNode(
-            "bar.cc",
-            execRoot.getRelative(bar.getExecPath()),
-            digestUtil.computeAsUtf8("bar"),
-            false);
+        FileNode.createExecutable(
+            "bar.cc", execRoot.getRelative(bar.getExecPath()), digestUtil.computeAsUtf8("bar"));
     FileNode expectedBuzzNode =
-        new FileNode(
-            "buzz.cc",
-            execRoot.getRelative(buzz.getExecPath()),
-            digestUtil.computeAsUtf8("buzz"),
-            false);
+        FileNode.createExecutable(
+            "buzz.cc", execRoot.getRelative(buzz.getExecPath()), digestUtil.computeAsUtf8("buzz"));
     assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
     assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode);
     assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBarNode);

src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java

@@ -74,10 +74,12 @@ public void buildingATreeOfFilesShouldWork() throws Exception {
     assertThat(directoriesAtDepth(1, tree)).containsExactly("fizz");
     assertThat(directoriesAtDepth(2, tree)).isEmpty();
 
-    FileNode expectedFooNode = new FileNode("foo.cc", foo, digestUtil.computeAsUtf8("foo"), false);
-    FileNode expectedBarNode = new FileNode("bar.cc", bar, digestUtil.computeAsUtf8("bar"), false);
+    FileNode expectedFooNode =
+        FileNode.createExecutable("foo.cc", foo, digestUtil.computeAsUtf8("foo"));
+    FileNode expectedBarNode =
+        FileNode.createExecutable("bar.cc", bar, digestUtil.computeAsUtf8("bar"));
     FileNode expectedBuzzNode =
-        new FileNode("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"), false);
+        FileNode.createExecutable("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"));
     assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
     assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode, expectedBarNode);
     assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBuzzNode);

src/test/java/com/google/devtools/build/lib/remote/merkletree/MerkleTreeTest.java

@@ -87,13 +87,13 @@ public void buildMerkleTree() throws IOException {
 
     Directory fizzDir =
         Directory.newBuilder()
-            .addFiles(newFileNode("buzz.cc", digestUtil.computeAsUtf8("buzz"), false))
-            .addFiles(newFileNode("fizzbuzz.cc", digestUtil.computeAsUtf8("fizzbuzz"), false))
+            .addFiles(newFileNode("buzz.cc", digestUtil.computeAsUtf8("buzz"), true))
+            .addFiles(newFileNode("fizzbuzz.cc", digestUtil.computeAsUtf8("fizzbuzz"), true))
             .build();
     Directory srcsDir =
         Directory.newBuilder()
-            .addFiles(newFileNode("bar.cc", digestUtil.computeAsUtf8("bar"), false))
-            .addFiles(newFileNode("foo.cc", digestUtil.computeAsUtf8("foo"), false))
+            .addFiles(newFileNode("bar.cc", digestUtil.computeAsUtf8("bar"), true))
+            .addFiles(newFileNode("foo.cc", digestUtil.computeAsUtf8("foo"), true))
             .addDirectories(
                 DirectoryNode.newBuilder().setName("fizz").setDigest(digestUtil.compute(fizzDir)))
             .build();

src/test/shell/bazel/remote/remote_execution_test.sh

@@ -2194,26 +2194,70 @@ EOF
     @local_foo//:all
 }
 
-function test_remote_input_files_executable_bit() {
-  # Test that input files uploaded to remote executor have the same executable bit with local files. #12818
+function test_remote_cache_intermediate_outputs() {
+  # test that remote cache is hit when intermediate output is not executable
   touch WORKSPACE
   cat > BUILD <<'EOF'
+genrule(
+  name = "dep",
+  srcs = [],
+  outs = ["dep"],
+  cmd = "echo 'dep' > $@",
+)
+
 genrule(
   name = "test",
-  srcs = ["foo.txt", "bar.sh"],
-  outs = ["out.txt"],
-  cmd = "ls -l $(SRCS); touch $@",
+  srcs = [":dep"],
+  outs = ["out"],
+  cmd = "cat $(SRCS) > $@",
 )
 EOF
-  touch foo.txt bar.sh
-  chmod a+x bar.sh
 
   bazel build \
-    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_cache=grpc://localhost:${worker_port} \
+    //:test >& $TEST_log || fail "Failed to build //:test"
+
+  bazel clean
+
+  bazel build \
+    --remote_cache=grpc://localhost:${worker_port} \
+    //:test >& $TEST_log || fail "Failed to build //:test"
+
+  expect_log "2 remote cache hit"
+}
+
+function test_remote_cache_intermediate_outputs_toplevel() {
+  # test that remote cache is hit when intermediate output is not executable in remote download toplevel mode
+  touch WORKSPACE
+  cat > BUILD <<'EOF'
+genrule(
+  name = "dep",
+  srcs = [],
+  outs = ["dep"],
+  cmd = "echo 'dep' > $@",
+)
+
+genrule(
+  name = "test",
+  srcs = [":dep"],
+  outs = ["out"],
+  cmd = "cat $(SRCS) > $@",
+)
+EOF
+
+  bazel build \
+    --remote_cache=grpc://localhost:${worker_port} \
+    --remote_download_toplevel \
+    //:test >& $TEST_log || fail "Failed to build //:test"
+
+  bazel clean
+
+  bazel build \
+    --remote_cache=grpc://localhost:${worker_port} \
+    --remote_download_toplevel \
     //:test >& $TEST_log || fail "Failed to build //:test"
 
-  expect_log "-rwxr--r-- .* bar.sh"
-  expect_log "-rw-r--r-- .* foo.txt"
+  expect_log "2 remote cache hit"
 }
 
 function test_exclusive_tag() {