Don't reference an action filesystem from build events

src/main/java/com/google/devtools/build/lib/analysis/test/TestAttempt.java

@@ -20,6 +20,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType;
 import com.google.devtools.build.lib.buildeventstream.BuildEventContext;
 import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
@@ -29,6 +30,7 @@
 import com.google.devtools.build.lib.buildeventstream.BuildEventWithOrderConstraint;
 import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent;
 import com.google.devtools.build.lib.buildeventstream.PathConverter;
+import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants;
 import com.google.devtools.build.lib.runtime.BuildEventStreamerUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
@@ -38,6 +40,7 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import javax.annotation.Nullable;
 
 /** This event is raised whenever an individual test attempt is completed. */
 public class TestAttempt implements BuildEventWithOrderConstraint {
@@ -49,6 +52,15 @@ public class TestAttempt implements BuildEventWithOrderConstraint {
   private final int attempt;
   private final boolean lastAttempt;
   private final ImmutableMultimap<String, Path> files;
+
+  /**
+   * Metadata of the test log, if known.
+   *
+   * <p>The test log is a declared output of the test action, so with {@code
+   * --remote_download_minimal} it may not exist on the local filesystem. Carrying its metadata lets
+   * the build event artifact uploader report it without having to read it.
+   */
+  @Nullable private final FileArtifactValue testLogMetadata;
   private final List<String> testWarnings;
   private final long durationMillis;
   private final long startTimeMillis;
@@ -71,6 +83,7 @@ private TestAttempt(
       long startTimeMillis,
       long durationMillis,
       ImmutableMultimap<String, Path> files,
+      @Nullable FileArtifactValue testLogMetadata,
       List<String> testWarnings,
       boolean lastAttempt) {
     this.testAction = testAction;
@@ -82,6 +95,7 @@ private TestAttempt(
     this.startTimeMillis = startTimeMillis;
     this.durationMillis = durationMillis;
     this.files = Preconditions.checkNotNull(files);
+    this.testLogMetadata = testLogMetadata;
     this.testWarnings = Preconditions.checkNotNull(testWarnings);
     this.lastAttempt = lastAttempt;
   }
@@ -95,6 +109,7 @@ public static TestAttempt forExecutedTestResult(
       TestResultData attemptData,
       int attempt,
       ImmutableMultimap<String, Path> files,
+      @Nullable FileArtifactValue testLogMetadata,
       BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo,
       boolean lastAttempt) {
     return new TestAttempt(
@@ -107,6 +122,7 @@ public static TestAttempt forExecutedTestResult(
         attemptData.getStartTimeMillisEpoch(),
         attemptData.getRunDurationMillis(),
         files,
+        testLogMetadata,
         attemptData.getWarningList(),
         lastAttempt);
   }
@@ -132,6 +148,7 @@ public static TestAttempt fromCachedTestResult(
         attemptData.getStartTimeMillisEpoch(),
         attemptData.getRunDurationMillis(),
         files,
+        /* testLogMetadata= */ null,
         attemptData.getWarningList(),
         lastAttempt);
   }
@@ -155,6 +172,7 @@ public static TestAttempt forUnstartableTestResult(
         attemptData.getStartTimeMillisEpoch(),
         attemptData.getRunDurationMillis(),
         /* files= */ ImmutableMultimap.of(),
+        /* testLogMetadata= */ null,
         attemptData.getWarningList(),
         /* lastAttempt= */ true);
   }
@@ -230,8 +248,13 @@ public ImmutableList<LocalFile> referencedLocalFiles() {
     ImmutableList.Builder<LocalFile> localFiles = ImmutableList.builder();
     for (Map.Entry<String, Path> file : files.entries()) {
       if (file.getValue() != null) {
-        // TODO(b/199940216): Can we populate metadata for these files?
-        localFiles.add(new LocalFile(file.getValue(), localFileType, /* artifactMetadata= */ null));
+        // Only the test log and the coverage data are declared outputs of the test action; the
+        // remaining files are produced by the test spawn and always exist locally.
+        localFiles.add(
+            new LocalFile(
+                file.getValue(),
+                localFileType,
+                TestFileNameConstants.TEST_LOG.equals(file.getKey()) ? testLogMetadata : null));
       }
     }
     return localFiles.build();

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

@@ -413,6 +413,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:action_input",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
+        "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
@@ -425,6 +426,7 @@ java_library(
         "//src/main/protobuf:failure_details_java_proto",
         "//src/main/protobuf:test_status_java_proto",
         "//third_party:guava",
+        "//third_party:jsr305",
         "@com_google_protobuf//:protobuf_java",
         "@com_google_protobuf//:protobuf_java_util",
     ],

src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java

@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ExecutionRequirements;
@@ -58,6 +59,7 @@
 import com.google.devtools.build.lib.server.FailureDetails.TestAction;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileStatus;
+import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -73,6 +75,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
+import javax.annotation.Nullable;
 
 /** Runs TestRunnerAction actions. */
 // TODO(bazel-team): add tests for this strategy.
@@ -242,6 +245,42 @@ private void finalizeTest(
     postTestResult(actionExecutionContext, result);
   }
 
+  /**
+   * Returns {@code testOutputs} with every path on the filesystem of {@code execRoot}.
+   *
+   * <p>The test log and the coverage data are declared outputs of the test action, so they are
+   * resolved through the action filesystem. That filesystem is scoped to the action, while the
+   * paths below are reported in build events, which are uploaded asynchronously and thus outlive
+   * it. See https://github.com/bazelbuild/bazel/issues/24527.
+   */
+  private static ImmutableMultimap<String, Path> onFileSystemOf(
+      ImmutableMultimap<String, Path> testOutputs, Path execRoot) {
+    FileSystem fileSystem = execRoot.getFileSystem();
+    ImmutableMultimap.Builder<String, Path> builder = ImmutableMultimap.builder();
+    testOutputs.forEach((name, path) -> builder.put(name, fileSystem.getPath(path.asFragment())));
+    return builder.build();
+  }
+
+  /**
+   * Returns the metadata of the test log, or null if it is unavailable.
+   *
+   * <p>The test log is a declared output of the test action, so with {@code
+   * --remote_download_minimal} it may not exist on the local filesystem. Its metadata lets the build
+   * event artifact uploader report it without reading it.
+   */
+  @Nullable
+  private static FileArtifactValue getTestLogMetadata(
+      ActionExecutionContext actionExecutionContext, TestRunnerAction action) {
+    try {
+      return actionExecutionContext.getOutputMetadataStore().getOutputMetadata(action.getTestLog());
+    } catch (IOException | InterruptedException e) {
+      if (e instanceof InterruptedException) {
+        Thread.currentThread().interrupt();
+      }
+      return null;
+    }
+  }
+
   private StandaloneProcessedAttemptResult processTestAttempt(
       int attemptId,
       boolean isLastAttempt,
@@ -283,13 +322,21 @@ private StandaloneProcessedAttemptResult processTestAttempt(
 
     // Add the test log to the output
     TestResultData data = dataBuilder.build();
+    ImmutableMultimap<String, Path> reportedOutputs =
+        onFileSystemOf(testOutputs, actionExecutionContext.getExecRoot());
     actionExecutionContext
         .getEventHandler()
         .post(
             TestAttempt.forExecutedTestResult(
-                action, data, attemptId, testOutputs, result.executionInfo(), isLastAttempt));
+                action,
+                data,
+                attemptId,
+                reportedOutputs,
+                getTestLogMetadata(actionExecutionContext, action),
+                result.executionInfo(),
+                isLastAttempt));
     processTestOutput(actionExecutionContext, data, action.getTestName(), renamedTestLog);
-    return new StandaloneProcessedAttemptResult(data, testOutputs);
+    return new StandaloneProcessedAttemptResult(data, reportedOutputs);
   }
 
   private static Map<String, String> setupEnvironment(
@@ -543,7 +590,12 @@ public TestResult newCachedTestResult(
       TestResultData cachedResult,
       ImmutableMultimap<String, Path> testOutputs) {
     return new TestResult(
-        action, cachedResult, testOutputs, /* cached= */ true, execRoot, /* systemFailure= */ null);
+        action,
+        cachedResult,
+        onFileSystemOf(testOutputs, execRoot),
+        /* cached= */ true,
+        execRoot,
+        /* systemFailure= */ null);
   }
 
   @VisibleForTesting

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

@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.remote;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
 import static com.google.devtools.build.lib.remote.util.DigestUtil.isOldStyleDigestFunction;
 import static com.google.devtools.build.lib.remote.util.RxFutures.toCompletable;
@@ -30,6 +31,7 @@
 import com.google.common.collect.Maps;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.buildeventstream.BuildEvent;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType;
 import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader;
@@ -109,9 +111,20 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
   }
 
   /** Returns {@code true} if Bazel knows that the file is stored on a remote system. */
-  private static boolean isRemoteFile(Path file) throws IOException {
-    return file.getFileSystem() instanceof RemoteActionFileSystem
-        && ((RemoteActionFileSystem) file.getFileSystem()).isRemote(file);
+  /**
+   * Checks that a build event does not reference an action filesystem.
+   *
+   * <p>A build event outlives the action it refers to, since it is uploaded asynchronously, so
+   * referencing one keeps everything it needs to serve that action's inputs on the heap until the
+   * event has been delivered. Events report paths on the local filesystem and supply {@link
+   * BuildEvent.LocalFile#artifactMetadata} for files that may not exist there. See
+   * https://github.com/bazelbuild/bazel/issues/24527.
+   */
+  private static void checkNotActionFileSystem(Path path) {
+    checkArgument(
+        !(path.getFileSystem() instanceof RemoteActionFileSystem),
+        "build event references an action filesystem: %s",
+        path);
   }
 
   private static final class PathMetadata {
@@ -248,7 +261,8 @@ private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOExcept
         digest,
         /* directory= */ false,
         /* symlink= */ false,
-        isRemoteFile(path),
+        // A file that is only stored remotely is reported through its metadata above.
+        /* remote= */ false,
         isBuildToolLog,
         digestFunction);
   }
@@ -392,6 +406,8 @@ private Single<PathConverter> doUpload(Map<Path, LocalFile> files) {
       return Single.just(PathConverter.NO_CONVERSION);
     }
 
+    files.keySet().forEach(ByteStreamBuildEventArtifactUploader::checkNotActionFileSystem);
+
     RequestMetadata metadata =
         TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "bes-upload");
     RemoteActionExecutionContext context =

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

@@ -20,9 +20,7 @@
 import com.google.common.eventbus.Subscribe;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
-import com.google.devtools.build.lib.actions.ActionInputMap;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.InputMetadataProvider;
 import com.google.devtools.build.lib.actions.LostInputsActionExecutionException;
 import com.google.devtools.build.lib.actions.OutputChecker;
@@ -209,24 +207,6 @@ public void clean() {
     // Intentionally left empty.
   }
 
-  @Override
-  public boolean supportsPathResolverForArtifactValues() {
-    return actionFileSystemType() != ActionFileSystemType.DISABLED;
-  }
-
-  @Override
-  public ArtifactPathResolver createPathResolverForArtifactValues(
-      PathFragment execRoot,
-      String relativeOutputPath,
-      FileSystem fileSystem,
-      ImmutableList<Root> pathEntries,
-      ActionInputMap actionInputMap) {
-    FileSystem remoteFileSystem =
-        new RemoteActionFileSystem(
-            fileSystem, execRoot, relativeOutputPath, actionInputMap, actionInputFetcher);
-    return ArtifactPathResolver.createPathResolver(remoteFileSystem, fileSystem.getPath(execRoot));
-  }
-
   @Override
   public void checkActionFileSystemForLostInputs(FileSystem actionFileSystem, Action action)
       throws LostInputsActionExecutionException {

src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java

@@ -509,7 +509,7 @@ private void maybeReportArtifactSet(CompletionContext ctx, NestedSet<?> set) {
       for (NestedSet<?> succ : set.getNonLeaves()) {
         maybeReportArtifactSet(ctx, succ);
       }
-      post(new NamedArtifactGroup(lockedName.getName(), ctx, set));
+      post(new NamedArtifactGroup(lockedName.getName(), ctx.pathResolver(), set));
     }
   }
 

src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java

@@ -19,6 +19,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.CompletionContext;
 import com.google.devtools.build.lib.actions.CompletionContext.ArtifactReceiver;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
@@ -45,16 +46,16 @@
  */
 class NamedArtifactGroup implements BuildEvent {
   private final String name;
-  private final CompletionContext completionContext;
+  private final ArtifactPathResolver pathResolver;
   private final NestedSet<?> set; // of Artifact or ExpandedArtifact
 
   /**
    * Create a {@link NamedArtifactGroup}. Although the set may contain a mixture of Artifacts and
    * ExpandedArtifacts, all its leaf successors ("direct elements") are ExpandedArtifacts.
    */
-  NamedArtifactGroup(String name, CompletionContext completionContext, NestedSet<?> set) {
+  NamedArtifactGroup(String name, ArtifactPathResolver pathResolver, NestedSet<?> set) {
     this.name = name;
-    this.completionContext = completionContext;
+    this.pathResolver = pathResolver;
     this.set = set;
   }
 
@@ -76,14 +77,14 @@ public Collection<LocalFile> referencedLocalFiles() {
         case NormalExpandedArtifact(Artifact artifact, FileArtifactValue metadata) -> {
           artifacts.add(
               new LocalFile(
-                  completionContext.pathResolver().toPath(artifact),
+                  pathResolver.toPath(artifact),
                   LocalFileType.forArtifact(artifact, metadata),
                   metadata));
         }
         case FilesetExpandedArtifact(Artifact fileset, FilesetOutputSymlink link) -> {
           artifacts.add(
               new LocalFile(
-                  completionContext.pathResolver().toPath(link.target()),
+                  pathResolver.toPath(link.target()),
                   LocalFileType.forArtifact(link.target(), link.metadata()),
                   link.metadata()));
         }
@@ -103,12 +104,11 @@ public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext convert
       BuildEventStreamProtos.File file =
           switch ((ExpandedArtifact) elem) {
             case NormalExpandedArtifact(Artifact artifact, FileArtifactValue metadata) -> {
-              String uri = pathConverter.apply(completionContext.pathResolver().toPath(artifact));
+              String uri = pathConverter.apply(pathResolver.toPath(artifact));
               yield TargetCompleteEvent.newFile(artifact, metadata, uri);
             }
             case FilesetExpandedArtifact(Artifact fileset, FilesetOutputSymlink link) -> {
-              String uri =
-                  pathConverter.apply(completionContext.pathResolver().toPath(link.target()));
+              String uri = pathConverter.apply(pathResolver.toPath(link.target()));
               yield TargetCompleteEvent.newFile(
                   fileset.getRoot(),
                   fileset.getRootRelativePath().getRelative(link.name()),