Fix checking remote cache for omitted files in buildevent file (bazelbuild#15405)

Enabling both `--noremote_upload_local_results` and
`--incompatible_remote_build_event_upload_respect_no_cache` caused
ByteStreamBuildEventArtifactuploader not to check if some files already
existed remotely because local files were not to be uploaded. When using
remote execution some of these files might exist remotely, so we want to
check remote cache for those files even if we would not upload them if
missing. The test case included depicts this failure case.

Closes bazelbuild#15280.

PiperOrigin-RevId: 442798312
(cherry picked from commit 317375d)

Co-authored-by: Emil Kattainen <[email protected]>

Author: brentleyjones

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

@@ -107,12 +107,14 @@ private static final class PathMetadata {
     private final Digest digest;
     private final boolean directory;
     private final boolean remote;
+    private final boolean omitted;
 
-    PathMetadata(Path path, Digest digest, boolean directory, boolean remote) {
+    PathMetadata(Path path, Digest digest, boolean directory, boolean remote, boolean omitted) {
       this.path = path;
       this.digest = digest;
       this.directory = directory;
       this.remote = remote;
+      this.omitted = omitted;
     }
 
     public Path getPath() {
@@ -130,6 +132,10 @@ public boolean isDirectory() {
     public boolean isRemote() {
       return remote;
     }
+
+    public boolean isOmitted() {
+      return omitted;
+    }
   }
 
   /**
@@ -138,22 +144,29 @@ public boolean isRemote() {
    */
   private PathMetadata readPathMetadata(Path file) throws IOException {
     if (file.isDirectory()) {
-      return new PathMetadata(file, /* digest= */ null, /* directory= */ true, /* remote= */ false);
-    }
-    if (omittedFiles.contains(file)) {
-      return new PathMetadata(file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false);
+      return new PathMetadata(
+          file,
+          /* digest= */ null,
+          /* directory= */ true,
+          /* remote= */ false,
+          /* omitted= */ false);
     }
 
-    for (Path treeRoot : omittedTreeRoots) {
-      if (file.startsWith(treeRoot)) {
-        omittedFiles.add(file);
-        return new PathMetadata(file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false);
+    boolean omitted = false;
+    if (omittedFiles.contains(file)) {
+      omitted = true;
+    } else {
+      for (Path treeRoot : omittedTreeRoots) {
+        if (file.startsWith(treeRoot)) {
+          omittedFiles.add(file);
+          omitted = true;
+        }
       }
     }
 
     DigestUtil digestUtil = new DigestUtil(file.getFileSystem().getDigestFunction());
     Digest digest = digestUtil.compute(file);
-    return new PathMetadata(file, digest, /* directory= */ false, isRemoteFile(file));
+    return new PathMetadata(file, digest, /* directory= */ false, isRemoteFile(file), omitted);
   }
 
   private static void processQueryResult(
@@ -166,14 +179,18 @@ private static void processQueryResult(
       } else {
         PathMetadata remotePathMetadata =
             new PathMetadata(
-                file.getPath(), file.getDigest(), file.isDirectory(), /* remote= */ true);
+                file.getPath(),
+                file.getDigest(),
+                file.isDirectory(),
+                /* remote= */ true,
+                file.isOmitted());
         knownRemotePaths.add(remotePathMetadata);
       }
     }
   }
 
   private static boolean shouldUpload(PathMetadata path) {
-    return path.getDigest() != null && !path.isRemote() && !path.isDirectory();
+    return path.getDigest() != null && !path.isRemote() && !path.isDirectory() && !path.isOmitted();
   }
 
   private Single<List<PathMetadata>> queryRemoteCache(
@@ -182,7 +199,8 @@ private Single<List<PathMetadata>> queryRemoteCache(
     List<PathMetadata> filesToQuery = new ArrayList<>();
     Set<Digest> digestsToQuery = new HashSet<>();
     for (PathMetadata path : paths) {
-      if (shouldUpload(path)) {
+      // Query remote cache for files even if omitted from uploading
+      if (shouldUpload(path) || path.isOmitted()) {
         filesToQuery.add(path);
         digestsToQuery.add(path.getDigest());
       } else {
@@ -239,7 +257,8 @@ private Single<List<PathMetadata>> uploadLocalFiles(
                                 path.getPath(),
                                 /*digest=*/ null,
                                 path.isDirectory(),
-                                path.isRemote()));
+                                path.isRemote(),
+                                path.isOmitted()));
                       });
             })
         .collect(Collectors.toList());
@@ -265,13 +284,17 @@ private Single<PathConverter> upload(Set<Path> files) {
                       } catch (IOException e) {
                         reporterUploadError(e);
                         return new PathMetadata(
-                            file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false);
+                            file,
+                            /*digest=*/ null,
+                            /*directory=*/ false,
+                            /*remote=*/ false,
+                            /*omitted=*/ false);
                       }
                     })
                 .collect(Collectors.toList())
                 .flatMap(paths -> queryRemoteCache(remoteCache, context, paths))
                 .flatMap(paths -> uploadLocalFiles(remoteCache, context, paths))
-                .map(paths -> new PathConverterImpl(remoteServerInstanceName, paths, omittedFiles)),
+                .map(paths -> new PathConverterImpl(remoteServerInstanceName, paths)),
         RemoteCache::release);
   }
 
@@ -305,23 +328,25 @@ private static class PathConverterImpl implements PathConverter {
     private final Set<Path> skippedPaths;
     private final Set<Path> localPaths;
 
-    PathConverterImpl(
-        String remoteServerInstanceName, List<PathMetadata> uploads, Set<Path> localPaths) {
+    PathConverterImpl(String remoteServerInstanceName, List<PathMetadata> uploads) {
       Preconditions.checkNotNull(uploads);
       this.remoteServerInstanceName = remoteServerInstanceName;
       pathToDigest = new HashMap<>(uploads.size());
       ImmutableSet.Builder<Path> skippedPaths = ImmutableSet.builder();
+      ImmutableSet.Builder<Path> localPaths = ImmutableSet.builder();
       for (PathMetadata pair : uploads) {
         Path path = pair.getPath();
         Digest digest = pair.getDigest();
-        if (digest != null) {
+        if (!pair.isRemote() && pair.isOmitted()) {
+          localPaths.add(path);
+        } else if (digest != null) {
           pathToDigest.put(path, digest);
         } else {
           skippedPaths.add(path);
         }
       }
       this.skippedPaths = skippedPaths.build();
-      this.localPaths = localPaths;
+      this.localPaths = localPaths.build();
     }
 
     @Override

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

@@ -3498,6 +3498,35 @@ EOF
   [[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote cas entries, not $remote_cas_files"
 }
 
+function test_remote_exec_convert_remote_file() {
+  mkdir -p a
+  cat > a/BUILD <<'EOF'
+sh_test(
+  name = "test",
+  srcs = ["test.sh"],
+)
+EOF
+  cat > a/test.sh <<'EOF'
+#!/bin/bash
+set -e
+echo \"foo\"
+exit 0
+EOF
+  chmod 755 a/test.sh
+
+  bazel test \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --build_event_json_file=bep.json \
+    --noremote_upload_local_results \
+    --incompatible_remote_build_event_upload_respect_no_cache \
+    //a:test >& $TEST_log || "Failed to test //a:test"
+
+  cat bep.json > $TEST_log
+  expect_not_log "test\.log.*file://" || fail "remote files generated in remote execution are not converted"
+  expect_log "test\.log.*bytestream://" || fail "remote files generated in remote execution are not converted"
+}
+
+
 function test_uploader_ignore_disk_cache_of_combined_cache() {
   mkdir -p a
   cat > a/BUILD <<EOF