Remote: Make chmod 0555 behavior consistent

After action execution, permission of output files is changed to [`0555`](bazelbuild#5588). This PR updates remote module in following ways to make the behavior consistent:

  1. Ignores `isExecutable` field of downloaded outputs from remote cache since the permission will be set to `0555` after action execution.
  2. Always set `isExecutable` to `true` instead of reading the real permission bits from file system when uploading local outputs.
  3. Do `chmod 0555` instead of `chmod 0755` when fetching inputs files for local actions which are outputs of previous remote actions. This should improve cache hit rate for builds that use dynamic execution and build without bytes.

Caveat: actions that depend on permission bits of input files (e.g. zip actions) shouldn't be executed dynamically since we have no control of input file permissions when running remotely. They should be always executed either locally or remotely.

b/198297058

Closes bazelbuild#13980.

PiperOrigin-RevId: 397257861

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

@@ -173,9 +173,12 @@ private Completable downloadFileAsync(Path path, FileArtifactValue metadata) {
 
   private void finalizeDownload(Path path) {
     try {
-      path.chmod(0755);
+      // The permission of output file is changed to 0555 after action execution. We manually change
+      // the permission here
+      // for the downloaded file to keep this behaviour consistent.
+      path.chmod(0555);
     } catch (IOException e) {
-      logger.atWarning().withCause(e).log("Failed to chmod 755 on %s", path);
+      logger.atWarning().withCause(e).log("Failed to chmod 555 on %s", path);
     }
   }
 

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

@@ -648,10 +648,11 @@ private void moveOutputsToFinalLocation(List<ListenableFuture<FileMetadata>> dow
      */
     Collections.sort(finishedDownloads, Comparator.comparing(f -> toTmpDownloadPath(f.path())));
 
-    // Move the output files from their temporary name to the actual output file name.
+    // Move the output files from their temporary name to the actual output file name. Executable
+    // bit
+    // is ignored since the file permission will be changed to 0555 after execution.
     for (FileMetadata outputFile : finishedDownloads) {
       FileSystemUtils.moveFile(toTmpDownloadPath(outputFile.path()), outputFile.path());
-      outputFile.path().setExecutable(outputFile.isExecutable());
     }
   }
 

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

@@ -244,7 +244,8 @@ private void addFile(Digest digest, Path file) throws IOException {
         .addOutputFilesBuilder()
         .setPath(remotePathResolver.localPathToOutputPath(file))
         .setDigest(digest)
-        .setIsExecutable(file.isExecutable());
+        // The permission of output file is changed to 0555 after action execution
+        .setIsExecutable(true);
 
     digestToFile.put(digest, file);
   }

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

@@ -453,7 +453,13 @@ public void updateActionResult(
 
     ActionResult result = uploadDirectory(remoteCache, ImmutableList.<Path>of(fooFile, barDir));
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
-    expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
+    // output files will have permission 0555 after action execution regardless the current
+    // permission
+    expectedResult
+        .addOutputFilesBuilder()
+        .setPath("a/foo")
+        .setDigest(fooDigest)
+        .setIsExecutable(true);
     expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest);
     assertThat(result).isEqualTo(expectedResult.build());
   }
@@ -719,7 +725,13 @@ public void updateActionResult(
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.setStdoutDigest(stdoutDigest);
     expectedResult.setStderrDigest(stderrDigest);
-    expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
+    // output files will have permission 0555 after action execution regardless the current
+    // permission
+    expectedResult
+        .addOutputFilesBuilder()
+        .setPath("a/foo")
+        .setDigest(fooDigest)
+        .setIsExecutable(true);
     expectedResult
         .addOutputFilesBuilder()
         .setPath("bar")
@@ -778,7 +790,13 @@ public void updateActionResult(
             command,
             ImmutableList.of(fooFile, barFile));
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
-    expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
+    // output files will have permission 0555 after action execution regardless the current
+    // permission
+    expectedResult
+        .addOutputFilesBuilder()
+        .setPath("a/foo")
+        .setDigest(fooDigest)
+        .setIsExecutable(true);
     expectedResult
         .addOutputFilesBuilder()
         .setPath("bar")
@@ -828,9 +846,11 @@ public void findMissingBlobs(
           }
         });
     ActionResult.Builder rb = ActionResult.newBuilder();
-    rb.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest);
+    // output files will have permission 0555 after action execution regardless the current
+    // permission
+    rb.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest).setIsExecutable(true);
     rb.addOutputFilesBuilder().setPath("bar").setDigest(barDigest).setIsExecutable(true);
-    rb.addOutputFilesBuilder().setPath("baz").setDigest(bazDigest);
+    rb.addOutputFilesBuilder().setPath("baz").setDigest(bazDigest).setIsExecutable(true);
     ActionResult result = rb.build();
     serviceRegistry.addService(
         new ActionCacheImplBase() {

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

@@ -214,7 +214,7 @@ public void testDownloadFile() throws Exception {
         .isEqualTo("hello world");
     assertThat(a1.getPath().isExecutable()).isTrue();
     assertThat(a1.getPath().isReadable()).isTrue();
-    assertThat(a1.getPath().isWritable()).isTrue();
+    assertThat(a1.getPath().isWritable()).isFalse();
   }
 
   @Test

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

@@ -163,8 +163,10 @@ public final void setUp() throws Exception {
   }
 
   @Test
-  public void downloadOutputs_outputFiles_maintainsExecutableBit() throws Exception {
-    // Test that downloading output files maintains executable bit works.
+  public void downloadOutputs_outputFiles_executableBitIgnored() throws Exception {
+    // Test that executable bit of downloaded output files are ignored since it will be chmod 555
+    // after action
+    // execution.
 
     // arrange
     Digest fooDigest = cache.addContents(remoteActionExecutionContext, "foo-contents");
@@ -190,7 +192,7 @@ public void downloadOutputs_outputFiles_maintainsExecutableBit() throws Exceptio
     assertThat(digestUtil.compute(execRoot.getRelative("outputs/foo"))).isEqualTo(fooDigest);
     assertThat(digestUtil.compute(execRoot.getRelative("outputs/bar"))).isEqualTo(barDigest);
     assertThat(execRoot.getRelative("outputs/foo").isExecutable()).isFalse();
-    assertThat(execRoot.getRelative("outputs/bar").isExecutable()).isTrue();
+    assertThat(execRoot.getRelative("outputs/bar").isExecutable()).isFalse();
     assertThat(context.isLockOutputFilesCalled()).isTrue();
   }
 
@@ -255,7 +257,7 @@ public void downloadOutputs_outputDirectories_works() throws Exception {
     // assert
     assertThat(digestUtil.compute(execRoot.getRelative("outputs/a/foo"))).isEqualTo(fooDigest);
     assertThat(digestUtil.compute(execRoot.getRelative("outputs/a/bar/qux"))).isEqualTo(quxDigest);
-    assertThat(execRoot.getRelative("outputs/a/bar/qux").isExecutable()).isTrue();
+    assertThat(execRoot.getRelative("outputs/a/bar/qux").isExecutable()).isFalse();
     assertThat(context.isLockOutputFilesCalled()).isTrue();
   }
 
@@ -1141,7 +1143,11 @@ public void uploadOutputs_uploadDirectory_works() throws Exception {
 
     // assert
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
-    expectedResult.addOutputFilesBuilder().setPath("outputs/a/foo").setDigest(fooDigest);
+    expectedResult
+        .addOutputFilesBuilder()
+        .setPath("outputs/a/foo")
+        .setDigest(fooDigest)
+        .setIsExecutable(true);
     expectedResult.addOutputDirectoriesBuilder().setPath("outputs/bar").setTreeDigest(barDigest);
     assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build());
 

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

@@ -77,7 +77,7 @@ public void actionResult_uploadSymlinks_absoluteFileSymlinkAsFile() throws Excep
     assertThat(um.getDigestToFile()).containsExactly(digest, link);
 
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
-    expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest);
+    expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest).setIsExecutable(true);
     assertThat(result.build()).isEqualTo(expectedResult.build());
   }
 
@@ -135,7 +135,7 @@ public void actionResult_noUploadSymlinks_relativeFileSymlinkAsFile() throws Exc
     assertThat(um.getDigestToFile()).containsExactly(digest, link);
 
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
-    expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest);
+    expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest).setIsExecutable(true);
     assertThat(result.build()).isEqualTo(expectedResult.build());
   }
 

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

@@ -3029,6 +3029,72 @@ EOF
   expect_log "ERROR: Failed to query remote execution capabilities: Failed to init TLS infrastructure using '/nope' as root certificate: File does not contain valid certificates: /nope"
 }
 
-run_suite "Remote execution and remote cache tests"
+function test_output_file_permission() {
+  # Test that permission of output files are always 0555
+
+  mkdir -p a
+  cat > a/BUILD <<EOF
+genrule(
+  name = "foo",
+  srcs = [],
+  outs = ["foo"],
+  cmd = "echo 'foo' > \$@",
+)
+
+genrule(
+  name = "bar",
+  srcs = [":foo"],
+  outs = ["bar"],
+  cmd = "ls -lL \$(SRCS) > \$@",
+  tags = ["no-remote"],
+)
+EOF
+
+  # no remote execution
+  bazel build \
+      //a:bar >& $TEST_log || fail "Failed to build"
+
+  ls -l bazel-bin/a/bar >& $TEST_log
+  expect_log "-r-xr-xr-x"
 
+  ls -l bazel-bin/a/foo >& $TEST_log
+  expect_log "-r-xr-xr-x"
+
+  cat bazel-bin/a/bar >& $TEST_log
+  expect_log "-r-xr-xr-x"
+
+  bazel clean >& $TEST_log || fail "Failed to clean"
+
+  # normal remote execution
+  bazel build \
+      --remote_executor=grpc://localhost:${worker_port} \
+      //a:bar >& $TEST_log || fail "Failed to build"
+
+  ls -l bazel-bin/a/bar >& $TEST_log
+  expect_log "-r-xr-xr-x"
+
+  ls -l bazel-bin/a/foo >& $TEST_log
+  expect_log "-r-xr-xr-x"
+
+  cat bazel-bin/a/bar >& $TEST_log
+  expect_log "-r-xr-xr-x"
+
+  bazel clean >& $TEST_log || fail "Failed to clean"
+
+  # build without bytes
+  bazel build \
+      --remote_executor=grpc://localhost:${worker_port} \
+      --remote_download_minimal \
+      //a:bar >& $TEST_log || fail "Failed to build"
+
+  ls -l bazel-bin/a/bar >& $TEST_log
+  expect_log "-r-xr-xr-x"
+
+  ls -l bazel-bin/a/foo >& $TEST_log
+  expect_log "-r-xr-xr-x"
+
+  cat bazel-bin/a/bar >& $TEST_log
+  expect_log "-r-xr-xr-x"
 }
+
+run_suite "Remote execution and remote cache tests"