From 601752991306aa4683cadf8a693cb06c3cdaf251 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 12 Aug 2026 18:40:54 +0200 Subject: [PATCH 1/4] Don't reference the action filesystem from test build events `TestAttempt` reported the test outputs as `Path`s on the test action's `RemoteActionFileSystem`. Build events are uploaded asynchronously, so that kept the filesystem - and with it the metadata of every input of the test action, including all of its runfiles - reachable until the transport had drained. Report them on the local filesystem instead. `TestRunnerAction` keeps resolving declared outputs through the path resolver, since an action filesystem may be the only place they exist; the conversion happens in `StandaloneTestStrategy`, which both of Bazel's paths funnel through - `processTestAttempt` for an executed test and `newCachedTestResult` for a locally cached one. Coverage data is always downloaded, but the test log is not with `--remote_download_minimal`, so `TestAttempt` now also carries its metadata; the build event artifact uploader prefers that over reading the file and reports the blob without downloading it. Progress towards #24527. --- .../build/lib/analysis/test/TestAttempt.java | 27 ++++++++- .../com/google/devtools/build/lib/exec/BUILD | 2 + .../lib/exec/StandaloneTestStrategy.java | 58 ++++++++++++++++++- .../com/google/devtools/build/lib/exec/BUILD | 1 + .../lib/exec/StandaloneTestStrategyTest.java | 49 +++++++++++++++- 5 files changed, 129 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestAttempt.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestAttempt.java index 3b2c2f522d2e00..528fde531a968b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestAttempt.java +++ b/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 files; + + /** + * Metadata of the test log, if known. + * + *

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 testWarnings; private final long durationMillis; private final long startTimeMillis; @@ -71,6 +83,7 @@ private TestAttempt( long startTimeMillis, long durationMillis, ImmutableMultimap files, + @Nullable FileArtifactValue testLogMetadata, List 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 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 referencedLocalFiles() { ImmutableList.Builder localFiles = ImmutableList.builder(); for (Map.Entry 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(); diff --git a/src/main/java/com/google/devtools/build/lib/exec/BUILD b/src/main/java/com/google/devtools/build/lib/exec/BUILD index 895b033a0f388f..f60ad88af31535 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/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", ], diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index 51558c98a3d629..c0e5707e41365c 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/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}. + * + *

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 onFileSystemOf( + ImmutableMultimap testOutputs, Path execRoot) { + FileSystem fileSystem = execRoot.getFileSystem(); + ImmutableMultimap.Builder 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. + * + *

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 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 setupEnvironment( @@ -543,7 +590,12 @@ public TestResult newCachedTestResult( TestResultData cachedResult, ImmutableMultimap testOutputs) { return new TestResult( - action, cachedResult, testOutputs, /* cached= */ true, execRoot, /* systemFailure= */ null); + action, + cachedResult, + onFileSystemOf(testOutputs, execRoot), + /* cached= */ true, + execRoot, + /* systemFailure= */ null); } @VisibleForTesting diff --git a/src/test/java/com/google/devtools/build/lib/exec/BUILD b/src/test/java/com/google/devtools/build/lib/exec/BUILD index 239584fb387063..43b456c19e0d92 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/test/java/com/google/devtools/build/lib/exec/BUILD @@ -37,6 +37,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:virtual_action_input", "//src/main/java/com/google/devtools/build/lib/analysis:actions/symlink_action", "//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/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:server_directories", diff --git a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java index 3d9596984020f7..d642db9fd6c19c 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.Iterables; import com.google.common.collect.MoreCollectors; import com.google.devtools.build.lib.actions.ActionContext; @@ -36,17 +37,17 @@ import com.google.devtools.build.lib.actions.DiscoveredModulesPruner; import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.Spawn; -import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnResult.Status; +import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnStrategy; import com.google.devtools.build.lib.actions.ThreadStateReceiver; import com.google.devtools.build.lib.actions.cache.OutputMetadataStore; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredTarget; -import com.google.devtools.build.lib.analysis.test.TestActionContext; import com.google.devtools.build.lib.analysis.test.TestActionContext.AttemptGroup; import com.google.devtools.build.lib.analysis.test.TestActionContext.ProcessedAttemptResult; import com.google.devtools.build.lib.analysis.test.TestActionContext.TestRunnerSpawn; +import com.google.devtools.build.lib.analysis.test.TestActionContext; import com.google.devtools.build.lib.analysis.test.TestAttempt; import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions.CancelConcurrentTests; import com.google.devtools.build.lib.analysis.test.TestProvider; @@ -56,6 +57,7 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.TestResult.ExecutionInfo; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.TestStatus; +import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.clock.Clock; import com.google.devtools.build.lib.events.Event; @@ -66,15 +68,17 @@ import com.google.devtools.build.lib.exec.util.FakeActionInputFileCache; import com.google.devtools.build.lib.exec.util.TestExecutorBuilder; import com.google.devtools.build.lib.runtime.TestSummaryOptions; -import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code; +import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.io.FileOutErr; +import com.google.devtools.build.lib.vfs.DigestHashFunction; 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.SyscallCache; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus; import com.google.devtools.build.lib.view.test.TestStatus.TestResultData; import com.google.devtools.common.options.Options; @@ -225,6 +229,45 @@ private static FileOutErr createTempOutErr(Path tmpDirRoot) { return new FileOutErr(outPath, errPath); } + @Test + public void cachedTestResult_outputsNotOnActionFileSystem() throws Exception { + ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class); + Path tmpDirRoot = TestStrategy.getTmpRoot(rootDirectory, outputBase, executionOptions); + TestedStandaloneTestStrategy standaloneTestStrategy = + new TestedStandaloneTestStrategy(executionOptions, TestSummaryOptions.DEFAULTS, tmpDirRoot); + scratch.file("standalone/simple_test.sh", "this does not get executed, it is mocked out"); + scratch.file( + "standalone/BUILD", + """ + load('//test_defs:foo_test.bzl', 'foo_test') + foo_test( + name = "simple_test", + size = "small", + srcs = ["simple_test.sh"], + ) + """); + TestRunnerAction testRunnerAction = getTestAction("//standalone:simple_test"); + + // A declared test output is resolved through the action filesystem, which is scoped to the + // action, so it must not end up in the reported outputs. See #24527. + FileSystem actionFileSystem = new InMemoryFileSystem(DigestHashFunction.SHA256); + Path testLogOnActionFileSystem = + actionFileSystem.getPath(testRunnerAction.getTestLog().getPath().asFragment()); + + TestResult result = + standaloneTestStrategy.newCachedTestResult( + rootDirectory, + testRunnerAction, + TestResultData.newBuilder().setTestPassed(true).build(), + ImmutableMultimap.of(TestFileNameConstants.TEST_LOG, testLogOnActionFileSystem)); + + TestAttempt attempt = Iterables.getOnlyElement(result.getCachedTestAttempts()); + Path reported = + Iterables.getOnlyElement(attempt.getFiles().get(TestFileNameConstants.TEST_LOG)); + assertThat(reported.asFragment()).isEqualTo(testLogOnActionFileSystem.asFragment()); + assertThat(reported.getFileSystem()).isNotSameInstanceAs(actionFileSystem); + } + private TestRunnerAction getTestAction(String target) throws Exception { ConfiguredTarget configuredTarget = getConfiguredTarget(target); ImmutableList testStatusArtifacts = From e1c4cf860245acb4932c8a7103eefa75f492d902 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 12 Aug 2026 18:07:08 +0200 Subject: [PATCH 2/4] Don't build BEP paths on an action filesystem in Bazel `CompletionContext` asks the `OutputService` for an `ArtifactPathResolver` so that `TargetCompleteEvent` and `NamedArtifactGroup` can turn artifacts into `Path`s. `RemoteOutputService` answered with a `RemoteActionFileSystem` wrapping the target's `ActionInputMap`. Since build events are uploaded asynchronously, that filesystem stayed reachable until the last transport had drained. It is no longer needed. Both events supply the artifact's `FileArtifactValue` alongside the path, and since 9a849d92d4 `ByteStreamBuildEventArtifactUploader` uses that metadata instead of stat'ing the file, so it never consults the filesystem to learn a digest, size or type. Drop the override and let the default `ArtifactPathResolver.IDENTITY` produce the same paths. The `OutputService` hook itself is left in place: other implementations, in particular those whose action filesystem takes full control of the output base, may still need to hand the build event stream a filesystem of their own. Also let `NamedArtifactGroup` retain just the `ArtifactPathResolver` instead of the whole `CompletionContext`. It stores eagerly expanded (artifact, metadata) pairs and consulted the context for nothing else, but there is one such event per nested set node of every target's output groups, so it was keeping the target's `ActionInputMap` alive along with it. Progress towards #24527. --- .../build/lib/remote/RemoteOutputService.java | 20 ------------------- .../build/lib/runtime/BuildEventStreamer.java | 2 +- .../build/lib/runtime/NamedArtifactGroup.java | 16 +++++++-------- 3 files changed, 9 insertions(+), 29 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java index 09edcf66524e4a..7c43df5bcdf886 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java +++ b/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 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 { diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java index b22dc64184bae8..74bc0541391fed 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java +++ b/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)); } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java b/src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java index c55bcd0da98178..e53e785017e9e7 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java +++ b/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 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()), From 6c177b79388349245dd79ba1881838c61f65fe72 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 12 Aug 2026 19:09:08 +0200 Subject: [PATCH 3/4] Don't reference the action filesystem from ActionExecutedEvent The primary output path and the stdout/stderr paths reported by `ActionExecutedEvent` came from the action's `ArtifactPathResolver`, so a published event kept the action filesystem alive until the transport had drained it. The stdout/stderr paths additionally made the action result upload read the action filesystem from a background thread with `--remote_cache_async`. Report them on the local filesystem instead, but only where that is known to work: an output service whose action filesystem takes full control of the output base has no local action log directory - `ExecutionTool` skips creating it when `supportsLocalActions()` is false - and its outputs may not exist locally either. For those, keep reporting the action filesystem paths. Progress towards #24527. --- .../lib/skyframe/SkyframeActionExecutor.java | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 956f51e6b9c3cd..059fc0a753058f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -1948,7 +1948,20 @@ private void informImportantOutputHandlerIfNecessary(FileOutErr outErr) { } } - private static void reportActionExecution( + /** + * Returns {@code path} on the filesystem that build events should reference. + * + *

A build event outlives the action it refers to, so it must not hold on to the action + * filesystem. That is only possible if the files also exist on the local filesystem, which is not + * the case for an output service whose action filesystem takes full control of the output base. + */ + private Path pathForBuildEvent(Path path) { + return outputService.actionFileSystemType().supportsLocalActions() + ? executorEngine.getExecRoot().getFileSystem().getPath(path.asFragment()) + : path; + } + + private void reportActionExecution( ExtendedEventHandler eventHandler, Path primaryOutputPath, @Nullable FileArtifactValue primaryOutputMetadata, @@ -1961,10 +1974,10 @@ private static void reportActionExecution( Path stderr = null; if (outErr.hasRecordedStdout()) { - stdout = outErr.getOutputPath(); + stdout = pathForBuildEvent(outErr.getOutputPath()); } if (outErr.hasRecordedStderr()) { - stderr = outErr.getErrorPath(); + stderr = pathForBuildEvent(outErr.getErrorPath()); } // Collect MetadataLogs and spawn start times/end times from the Action's SpawnResults. ImmutableList spawnResults = @@ -1985,7 +1998,7 @@ private static void reportActionExecution( action.getPrimaryOutput().getExecPath(), action, exception, - primaryOutputPath, + pathForBuildEvent(primaryOutputPath), action.getPrimaryOutput(), primaryOutputMetadata, stdout, From b85d86d95fc3acf94f3de637b337467078b045ff Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 12 Aug 2026 19:56:28 +0200 Subject: [PATCH 4/4] Enforce that build events don't reference an action filesystem `ByteStreamBuildEventArtifactUploader` is only used by Bazel, whose action filesystem is always `RemoteActionFileSystem`, so the invariant can be checked unconditionally there rather than having each filesystem opt in. The check fails hard rather than reporting a bug: `BugReport` returns early for a binary that isn't a released Blaze, and only logs otherwise, so a violation would go unnoticed in Bazel. With the invariant in place, the uploader no longer needs to ask an action filesystem whether a file is stored remotely: a build event that references such a file supplies its metadata, which already carries that information. Progress towards #24527. --- .../ByteStreamBuildEventArtifactUploader.java | 24 +++++++++++++++---- ...eStreamBuildEventArtifactUploaderTest.java | 23 +++++++----------- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java index 84861540650f4d..97b6e938920287 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java +++ b/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. + * + *

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 doUpload(Map files) { return Single.just(PathConverter.NO_CONVERSION); } + files.keySet().forEach(ByteStreamBuildEventArtifactUploader::checkNotActionFileSystem); + RequestMetadata metadata = TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "bes-upload"); RemoteActionExecutionContext context = diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index d4b3c745ac74fb..5494f3d580632e 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -413,9 +413,10 @@ public void onCompleted() { } @Test - public void remoteFileShouldNotBeUploaded_actionFs() throws Exception { - // Test that we don't attempt to upload remotely stored file but convert the remote path - // to a bytestream:// URI. + public void remoteFileShouldNotBeUploaded_remoteMetadata() throws Exception { + // Test that we don't attempt to upload a remotely stored file, but convert it to a + // bytestream:// URI. Build events report such a file on the local filesystem, where it does not + // exist, and supply its metadata; see #24527. // arrange @@ -430,23 +431,15 @@ public void remoteFileShouldNotBeUploaded_actionFs() throws Exception { ActionInputMap outputs = new ActionInputMap(2); Artifact artifact = createRemoteArtifact("file1.txt", "foo", outputs); - RemoteActionFileSystem remoteFs = - new RemoteActionFileSystem( - fs, - execRoot.asFragment(), - outputRoot.getRoot().asPath().relativeTo(execRoot).getPathString(), - outputs, - actionInputFetcher); - Path remotePath = remoteFs.getPath(artifact.getPath().getPathString()); - assertThat(remotePath.getFileSystem()).isEqualTo(remoteFs); - LocalFile file = - new LocalFile(remotePath, LocalFileType.OUTPUT_FILE, /* artifactMetadata= */ null); + FileArtifactValue metadata = outputs.getInputMetadata(artifact); + Path remotePath = artifact.getPath(); + assertThat(remotePath.exists()).isFalse(); + LocalFile file = new LocalFile(remotePath, LocalFileType.OUTPUT_FILE, metadata); // act PathConverter pathConverter = artifactUploader.upload(ImmutableMap.of(remotePath, file)).get(); - FileArtifactValue metadata = outputs.getInputMetadata(artifact); Digest digest = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize()); // assert