diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index 6f4943026810ed..0612e5109bfe8f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -144,19 +144,11 @@ private ActionCache.Entry getCacheEntry(Action action) { if (!cacheConfig.enabled()) { return null; // ignore existing cache when disabled. } - for (Artifact output : action.getOutputs()) { - ActionCache.Entry entry = actionCache.get(output.getExecPathString()); - if (entry != null) { - return entry; - } - } - return null; + return ActionCacheUtils.getCacheEntry(actionCache, action); } private void removeCacheEntry(Action action) { - for (Artifact output : action.getOutputs()) { - actionCache.remove(output.getExecPathString()); - } + ActionCacheUtils.removeCacheEntry(actionCache, action); } @Nullable diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheUtils.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheUtils.java new file mode 100644 index 00000000000000..49065220a98869 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheUtils.java @@ -0,0 +1,40 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.actions; + +import com.google.devtools.build.lib.actions.cache.ActionCache; +import javax.annotation.Nullable; + +/** Utility functions for {@link ActionCache}. */ +public class ActionCacheUtils { + private ActionCacheUtils() {} + + /** Checks whether one of existing output paths is already used as a key. */ + @Nullable + public static ActionCache.Entry getCacheEntry(ActionCache actionCache, Action action) { + for (Artifact output : action.getOutputs()) { + ActionCache.Entry entry = actionCache.get(output.getExecPathString()); + if (entry != null) { + return entry; + } + } + return null; + } + + public static void removeCacheEntry(ActionCache actionCache, Action action) { + for (Artifact output : action.getOutputs()) { + actionCache.remove(output.getExecPathString()); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index 6f2be02509b237..c79ffd2162daef 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -42,6 +42,7 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable; import com.google.devtools.build.lib.events.Reporter; +import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.util.AsyncTaskCache; import com.google.devtools.build.lib.remote.util.RxUtils.TransferResult; import com.google.devtools.build.lib.remote.util.TempPathGenerator; @@ -60,6 +61,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import java.util.regex.Pattern; +import javax.annotation.Nullable; /** * Abstract implementation of {@link ActionInputPrefetcher} which implements the orchestration of @@ -76,6 +78,8 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet protected final Path execRoot; protected final ImmutableList patternsToDownload; + private final Set missingActionInputs = Sets.newConcurrentHashSet(); + private static class Context { private final Set nonWritableDirs = Sets.newConcurrentHashSet(); @@ -387,7 +391,8 @@ private Completable prefetchInputFileOrSymlink( PathFragment prefetchExecPath = metadata.getMaterializationExecPath().orElse(execPath); Completable prefetch = - downloadFileNoCheckRx(context, execRoot.getRelative(prefetchExecPath), metadata, priority); + downloadFileNoCheckRx( + context, execRoot.getRelative(prefetchExecPath), input, metadata, priority); // If prefetching to a different path, plant a symlink into it. if (!prefetchExecPath.equals(execPath)) { @@ -406,15 +411,23 @@ private Completable prefetchInputFileOrSymlink( * download finished. */ private Completable downloadFileRx( - Context context, Path path, FileArtifactValue metadata, Priority priority) { + Context context, + Path path, + @Nullable ActionInput actionInput, + FileArtifactValue metadata, + Priority priority) { if (!canDownloadFile(path, metadata)) { return Completable.complete(); } - return downloadFileNoCheckRx(context, path, metadata, priority); + return downloadFileNoCheckRx(context, path, actionInput, metadata, priority); } private Completable downloadFileNoCheckRx( - Context context, Path path, FileArtifactValue metadata, Priority priority) { + Context context, + Path path, + @Nullable ActionInput actionInput, + FileArtifactValue metadata, + Priority priority) { if (path.isSymbolicLink()) { try { path = path.getRelative(path.readSymbolicLink()); @@ -428,26 +441,32 @@ private Completable downloadFileNoCheckRx( AtomicBoolean completed = new AtomicBoolean(false); Completable download = Completable.using( - tempPathGenerator::generateTempPath, - tempPath -> - toCompletable( - () -> - doDownloadFile( - tempPath, finalPath.relativeTo(execRoot), metadata, priority), - directExecutor()) - .doOnComplete( - () -> { - finalizeDownload(context, tempPath, finalPath); - completed.set(true); - }), - tempPath -> { - if (!completed.get()) { - deletePartialDownload(tempPath); - } - }, - // Set eager=false here because we want cleanup the download *after* upstream is - // disposed. - /* eager= */ false); + tempPathGenerator::generateTempPath, + tempPath -> + toCompletable( + () -> + doDownloadFile( + tempPath, finalPath.relativeTo(execRoot), metadata, priority), + directExecutor()) + .doOnComplete( + () -> { + finalizeDownload(context, tempPath, finalPath); + completed.set(true); + }), + tempPath -> { + if (!completed.get()) { + deletePartialDownload(tempPath); + } + }, + // Set eager=false here because we want cleanup the download *after* upstream is + // disposed. + /* eager= */ false) + .doOnError( + error -> { + if (error instanceof CacheNotFoundException && actionInput != null) { + missingActionInputs.add(actionInput); + } + }); return downloadCache.executeIfNot( finalPath, @@ -467,19 +486,27 @@ private Completable downloadFileNoCheckRx( *

The file will be written into a temporary file and moved to the final destination after the * download finished. */ - public void downloadFile(Path path, FileArtifactValue metadata) + public void downloadFile(Path path, @Nullable ActionInput actionInput, FileArtifactValue metadata) throws IOException, InterruptedException { - getFromFuture(downloadFileAsync(path.asFragment(), metadata, Priority.CRITICAL)); + getFromFuture(downloadFileAsync(path.asFragment(), actionInput, metadata, Priority.CRITICAL)); } protected ListenableFuture downloadFileAsync( - PathFragment path, FileArtifactValue metadata, Priority priority) { + PathFragment path, + @Nullable ActionInput actionInput, + FileArtifactValue metadata, + Priority priority) { Context context = new Context(); return toListenableFuture( Completable.using( () -> context, ctx -> - downloadFileRx(context, execRoot.getFileSystem().getPath(path), metadata, priority), + downloadFileRx( + context, + execRoot.getFileSystem().getPath(path), + actionInput, + metadata, + priority), Context::finalizeContext)); } @@ -636,4 +663,8 @@ private boolean outputMatchesPattern(Artifact output) { public void flushOutputTree() throws InterruptedException { downloadCache.awaitInProgressTasks(); } + + public ImmutableSet getMissingActionInputs() { + return ImmutableSet.copyOf(missingActionInputs); + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index dfbe7adc482023..c1c369c0b7acf4 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -52,6 +52,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib:runtime/command_line_path_factory", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper", + "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", "//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", @@ -183,11 +184,13 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/remote/common", "//src/main/java/com/google/devtools/build/lib/remote/util", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//third_party:flogger", "//third_party:guava", + "//third_party:jsr305", "//third_party:rxjava3", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index 4d59c2d73b75b0..b6a597e7ca1fff 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java @@ -24,6 +24,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; @@ -538,6 +539,12 @@ public RemoteFileArtifactValue getRemoteMetadata() { }; } + @Nullable + protected ActionInput getActionInput(PathFragment path) { + PathFragment execPath = path.relativeTo(execRoot); + return inputArtifactData.getInput(execPath.getPathString()); + } + @Nullable protected RemoteFileArtifactValue getRemoteMetadata(PathFragment path) { if (!isOutput(path)) { @@ -577,7 +584,7 @@ private void downloadFileIfRemote(PathFragment path) throws IOException { FileArtifactValue m = getRemoteMetadata(path); if (m != null) { try { - inputFetcher.downloadFile(delegateFs.getPath(path), m); + inputFetcher.downloadFile(delegateFs.getPath(path), getActionInput(path), m); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new IOException( diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index 43f9994623069a..f081add8ad9caa 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -151,15 +151,21 @@ public ListenableFuture uploadActionResult( */ public ListenableFuture uploadFile( RemoteActionExecutionContext context, Digest digest, Path file) { + return uploadFile(context, digest, file, /* force= */ false); + } + + protected ListenableFuture uploadFile( + RemoteActionExecutionContext context, Digest digest, Path file, boolean force) { if (digest.getSizeBytes() == 0) { return COMPLETED_SUCCESS; } Completable upload = - casUploadCache.executeIfNot( + casUploadCache.execute( digest, RxFutures.toCompletable( - () -> cacheProtocol.uploadFile(context, digest, file), directExecutor())); + () -> cacheProtocol.uploadFile(context, digest, file), directExecutor()), + force); return RxFutures.toListenableFuture(upload); } @@ -176,15 +182,21 @@ public ListenableFuture uploadFile( */ public ListenableFuture uploadBlob( RemoteActionExecutionContext context, Digest digest, ByteString data) { + return uploadBlob(context, digest, data, /* force= */ false); + } + + protected ListenableFuture uploadBlob( + RemoteActionExecutionContext context, Digest digest, ByteString data, boolean force) { if (digest.getSizeBytes() == 0) { return COMPLETED_SUCCESS; } Completable upload = - casUploadCache.executeIfNot( + casUploadCache.execute( digest, RxFutures.toCompletable( - () -> cacheProtocol.uploadBlob(context, digest, data), directExecutor())); + () -> cacheProtocol.uploadBlob(context, digest, data), directExecutor()), + force); return RxFutures.toListenableFuture(upload); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 701275a3e6de46..9d567bf755a2c6 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -34,7 +34,9 @@ import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; +import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.analysis.AnalysisResult; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -58,6 +60,7 @@ import com.google.devtools.build.lib.exec.ModuleActionContextRegistry; import com.google.devtools.build.lib.exec.SpawnStrategyRegistry; import com.google.devtools.build.lib.remote.RemoteServerCapabilities.ServerCapabilitiesRequirement; +import com.google.devtools.build.lib.remote.ToplevelArtifactsDownloader.PathToMetadataConverter; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; import com.google.devtools.build.lib.remote.downloader.GrpcRemoteDownloader; @@ -921,7 +924,6 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB remoteOptions.useNewExitCodeForLostInputs); env.getEventBus().register(actionInputFetcher); builder.setActionInputPrefetcher(actionInputFetcher); - remoteOutputService.setActionInputFetcher(actionInputFetcher); actionContextProvider.setActionInputFetcher(actionInputFetcher); toplevelArtifactsDownloader = @@ -930,15 +932,35 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB remoteOutputsMode.downloadToplevelOutputsOnly(), env.getSkyframeExecutor().getEvaluator(), actionInputFetcher, - (path) -> { - FileSystem fileSystem = path.getFileSystem(); - if (fileSystem instanceof RemoteActionFileSystem) { - return ((RemoteActionFileSystem) path.getFileSystem()) - .getRemoteMetadata(path.asFragment()); + new PathToMetadataConverter() { + @Nullable + @Override + public FileArtifactValue getMetadata(Path path) { + FileSystem fileSystem = path.getFileSystem(); + if (fileSystem instanceof RemoteActionFileSystem) { + return ((RemoteActionFileSystem) path.getFileSystem()) + .getRemoteMetadata(path.asFragment()); + } + return null; + } + + @Nullable + @Override + public ActionInput getActionInput(Path path) { + FileSystem fileSystem = path.getFileSystem(); + if (fileSystem instanceof RemoteActionFileSystem) { + return ((RemoteActionFileSystem) path.getFileSystem()) + .getActionInput(path.asFragment()); + } + return null; } - return null; }); env.getEventBus().register(toplevelArtifactsDownloader); + + remoteOutputService.setActionInputFetcher(actionInputFetcher); + remoteOutputService.setMemoizingEvaluator(env.getSkyframeExecutor().getEvaluator()); + remoteOutputService.setActionCache(env.getBlazeWorkspace().getPersistentActionCache()); + env.getEventBus().register(remoteOutputService); } } 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 a293b40408282f..f6c5f138a89dd0 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 @@ -18,13 +18,20 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.eventbus.Subscribe; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionCacheUtils; +import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputMap; +import com.google.devtools.build.lib.actions.ActionLookupData; +import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.FilesetOutputSymlink; +import com.google.devtools.build.lib.actions.cache.ActionCache; import com.google.devtools.build.lib.actions.cache.MetadataHandler; import com.google.devtools.build.lib.actions.cache.MetadataInjector; +import com.google.devtools.build.lib.buildtool.buildevent.ExecutionPhaseCompleteEvent; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.vfs.BatchStat; @@ -33,8 +40,10 @@ import com.google.devtools.build.lib.vfs.OutputService; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; +import com.google.devtools.build.skyframe.MemoizingEvaluator; import com.google.devtools.build.skyframe.SkyFunction.Environment; import java.io.IOException; +import java.util.HashMap; import java.util.Map; import java.util.UUID; import javax.annotation.Nullable; @@ -43,11 +52,21 @@ public class RemoteOutputService implements OutputService { @Nullable private RemoteActionInputFetcher actionInputFetcher; + @Nullable MemoizingEvaluator memoizingEvaluator; + @Nullable ActionCache actionCache; void setActionInputFetcher(RemoteActionInputFetcher actionInputFetcher) { this.actionInputFetcher = Preconditions.checkNotNull(actionInputFetcher, "actionInputFetcher"); } + void setMemoizingEvaluator(MemoizingEvaluator memoizingEvaluator) { + this.memoizingEvaluator = memoizingEvaluator; + } + + void setActionCache(ActionCache actionCache) { + this.actionCache = actionCache; + } + @Override public ActionFileSystemType actionFileSystemType() { return actionInputFetcher != null @@ -107,6 +126,49 @@ public void finalizeBuild(boolean buildSuccessful) { // Intentionally left empty. } + @Subscribe + public void onExecutionPhaseCompleteEvent(ExecutionPhaseCompleteEvent event) { + processMissingInputs(); + } + + private void processMissingInputs() { + if (memoizingEvaluator == null || actionInputFetcher == null) { + return; + } + + var actions = new HashMap(); + + try { + for (ActionInput actionInput : actionInputFetcher.getMissingActionInputs()) { + if (actionInput instanceof Artifact.DerivedArtifact) { + Artifact.DerivedArtifact output = (Artifact.DerivedArtifact) actionInput; + ActionLookupData actionLookupData = output.getGeneratingActionKey(); + var actionLookupValue = + memoizingEvaluator.getExistingValue(actionLookupData.getActionLookupKey()); + if (actionLookupValue instanceof ActionLookupValue) { + Action action = + ((ActionLookupValue) actionLookupValue) + .getAction(actionLookupData.getActionIndex()); + actions.put(actionLookupData, action); + } + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + + if (!actions.isEmpty()) { + var actionKeys = actions.keySet(); + memoizingEvaluator.delete(key -> key instanceof ActionLookupData && actionKeys.contains(key)); + + if (actionCache != null) { + for (var action : actions.values()) { + ActionCacheUtils.removeCacheEntry(actionCache, action); + } + } + } + } + @Override public void flushActionFileSystem(FileSystem actionFileSystem) throws IOException { ((RemoteActionFileSystem) actionFileSystem).flush(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java index 91574d8f9d5ebd..c266070a881374 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java @@ -107,6 +107,9 @@ public ToplevelArtifactsDownloader( public interface PathToMetadataConverter { @Nullable FileArtifactValue getMetadata(Path path); + + @Nullable + ActionInput getActionInput(Path path); } private void downloadTestOutput(Path path) { @@ -118,9 +121,11 @@ private void downloadTestOutput(Path path) { // action didn't get the chance to execute. In this case the metadata is null which is fine // because test outputs are already downloaded (otherwise it cannot hit the action cache). FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path); + ActionInput actionInput = pathToMetadataConverter.getActionInput(path); if (metadata != null) { ListenableFuture future = - actionInputPrefetcher.downloadFileAsync(path.asFragment(), metadata, Priority.LOW); + actionInputPrefetcher.downloadFileAsync( + path.asFragment(), actionInput, metadata, Priority.LOW); addCallback( future, new FutureCallback() { diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java index 5eb00a6048fb79..91999677fb4950 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java @@ -264,6 +264,12 @@ public ActionCache getOrLoadPersistentActionCache(Reporter reporter) throws IOEx return actionCache; } + /** Returns reference to the lazily instantiated persistent action cache instance */ + @Nullable + public ActionCache getPersistentActionCache() { + return actionCache; + } + /** * Generates a README file in the output base directory. This README file * contains the name of the workspace directory, so that users can figure out diff --git a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java index 912c269d6e9a3f..c9cc73b57da2b8 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java @@ -90,7 +90,7 @@ protected Artifact createRemoteArtifact( String contents, @Nullable PathFragment materializationExecPath, Map metadata, - Map cas) { + @Nullable Map cas) { Path p = artifactRoot.getRoot().getRelative(pathFragment); Artifact a = ActionsTestUtil.createArtifact(artifactRoot, p); byte[] contentsBytes = contents.getBytes(UTF_8); @@ -103,7 +103,9 @@ protected Artifact createRemoteArtifact( "action-id", materializationExecPath); metadata.put(a, f); - cas.put(hashCode, contentsBytes); + if (cas != null) { + cas.put(hashCode, contentsBytes); + } return a; } @@ -111,7 +113,7 @@ protected Artifact createRemoteArtifact( String pathFragment, String contents, Map metadata, - Map cas) { + @Nullable Map cas) { return createRemoteArtifact( pathFragment, contents, /* materializationExecPath= */ null, metadata, cas); } @@ -442,7 +444,7 @@ public void downloadFile_downloadRemoteFiles() throws Exception { Artifact a1 = createRemoteArtifact("file1", "hello world", metadata, cas); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - prefetcher.downloadFile(a1.getPath(), metadata.get(a1)); + prefetcher.downloadFile(a1.getPath(), /* actionInput= */ null, metadata.get(a1)); assertThat(FileSystemUtils.readContent(a1.getPath(), UTF_8)).isEqualTo("hello world"); assertThat(a1.getPath().isExecutable()).isTrue(); @@ -471,7 +473,7 @@ public void downloadFile_onInterrupt_deletePartialDownloadedFile() throws Except new Thread( () -> { try { - prefetcher.downloadFile(a1.getPath(), metadata.get(a1)); + prefetcher.downloadFile(a1.getPath(), /* actionInput= */ null, metadata.get(a1)); } catch (IOException ignored) { // Intentionally left empty } catch (InterruptedException e) { diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java index 3709b2c9003300..2e086170e34e81 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java @@ -453,7 +453,7 @@ public void remoteCacheEvictBlobs_exitWithCode39() throws Exception { assertOutputDoesNotExist("a/foo.out"); // Act: Evict blobs from remote cache and do an incremental build - getFileSystem().getPath(worker.getCasPath().getSafePathString()).deleteTreesBelow(); + worker.restart(); write("a/bar.in", "updated bar"); var error = assertThrows(BuildFailedException.class, () -> buildTarget("//a:bar")); @@ -466,4 +466,51 @@ public void remoteCacheEvictBlobs_exitWithCode39() throws Exception { assertThat(error).hasMessageThat().contains(String.format("%s/%s", hashCode, bytes.length)); assertThat(error.getDetailedExitCode().getExitCode().getNumericExitCode()).isEqualTo(39); } + + @Test + public void remoteCacheEvictBlobs_incrementalBuildCanContinue() throws Exception { + // Arrange: Prepare workspace and populate remote cache + write( + "a/BUILD", + "genrule(", + " name = 'foo',", + " srcs = ['foo.in'],", + " outs = ['foo.out'],", + " cmd = 'cat $(SRCS) > $@',", + ")", + "genrule(", + " name = 'bar',", + " srcs = ['foo.out', 'bar.in'],", + " outs = ['bar.out'],", + " cmd = 'cat $(SRCS) > $@',", + " tags = ['no-remote-exec'],", + ")"); + write("a/foo.in", "foo"); + write("a/bar.in", "bar"); + + // Populate remote cache + buildTarget("//a:bar"); + getOutputPath("a/foo.out").delete(); + getOutputPath("a/bar.out").delete(); + getOutputBase().getRelative("action_cache").deleteTreesBelow(); + restartServer(); + + // Clean build, foo.out isn't downloaded + buildTarget("//a:bar"); + assertOutputDoesNotExist("a/foo.out"); + + // Evict blobs from remote cache + worker.restart(); + + // trigger build error + write("a/bar.in", "updated bar"); + // Build failed because of remote cache eviction + assertThrows(BuildFailedException.class, () -> buildTarget("//a:bar")); + + // Act: Do an incremental build without "clean" or "shutdown" + buildTarget("//a:bar"); + + // Assert: target was successfully built + assertValidOutputFile("a/bar.out", "foo" + lineSeparator() + "updated bar" + lineSeparator()); + } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java index 6f9ac3600fbbff..0fd2d2d4639ea8 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java @@ -610,6 +610,127 @@ public void incrementalBuild_restartServer_hitActionCache() throws Exception { assertOutputsDoNotExist("//:foobar"); } + @Test + public void incrementalBuild_sourceModified_rerunActions() throws Exception { + // Arrange: Prepare workspace and run a clean build + write("foo.in", "foo"); + write( + "BUILD", + "genrule(", + " name = 'foo',", + " srcs = ['foo.in'],", + " outs = ['out/foo.txt'],", + " cmd = 'cat $(SRCS) > $@',", + ")", + "genrule(", + " name = 'foobar',", + " srcs = [':foo'],", + " outs = ['out/foobar.txt'],", + " cmd = 'cat $(location :foo) > $@ && echo bar >> $@',", + " tags = ['no-remote'],", + ")"); + + buildTarget("//:foobar"); + assertValidOutputFile("out/foo.txt", "foo" + lineSeparator()); + assertValidOutputFile("out/foobar.txt", "foo" + lineSeparator() + "bar\n"); + + // Act: Modify source file and run an incremental build + write("foo.in", "modified"); + + ActionEventCollector actionEventCollector = new ActionEventCollector(); + getRuntimeWrapper().registerSubscriber(actionEventCollector); + buildTarget("//:foobar"); + + // Assert: All actions transitively depend on the source file are re-executed and outputs are + // correct. + assertValidOutputFile("out/foo.txt", "modified" + lineSeparator()); + assertValidOutputFile("out/foobar.txt", "modified" + lineSeparator() + "bar\n"); + assertThat(actionEventCollector.getNumActionNodesEvaluated()).isEqualTo(2); + } + + @Test + public void incrementalBuild_intermediateOutputDeleted_nothingIsReEvaluated() throws Exception { + // Arrange: Prepare workspace and run a clean build + write( + "BUILD", + "genrule(", + " name = 'foo',", + " srcs = [],", + " outs = ['out/foo.txt'],", + " cmd = 'echo foo > $@',", + ")", + "genrule(", + " name = 'foobar',", + " srcs = [':foo'],", + " outs = ['out/foobar.txt'],", + " cmd = 'cat $(location :foo) > $@ && echo bar >> $@',", + " tags = ['no-remote'],", + ")"); + + buildTarget("//:foobar"); + assertValidOutputFile("out/foo.txt", "foo\n"); + assertValidOutputFile("out/foobar.txt", "foo\nbar\n"); + + // Act: Delete intermediate output and run an incremental build + var fooPath = getOutputPath("out/foo.txt"); + fooPath.delete(); + + ActionEventCollector actionEventCollector = new ActionEventCollector(); + getRuntimeWrapper().registerSubscriber(actionEventCollector); + buildTarget("//:foobar"); + + // Assert: local output is deleted, skyframe should trust remote files so no nodes will be + // re-evaluated. + assertOutputDoesNotExist("out/foo.txt"); + assertValidOutputFile("out/foobar.txt", "foo\nbar\n"); + assertThat(actionEventCollector.getNumActionNodesEvaluated()).isEqualTo(0); + } + + @Test + public void incrementalBuild_intermediateOutputModified_rerunGeneratingActions() + throws Exception { + // Arrange: Prepare workspace and run a clean build + write( + "BUILD", + "genrule(", + " name = 'foo',", + " srcs = [],", + " outs = ['out/foo.txt'],", + " cmd = 'echo foo > $@',", + ")", + "genrule(", + " name = 'foobar',", + " srcs = [':foo'],", + " outs = ['out/foobar.txt'],", + " cmd = 'cat $(location :foo) > $@ && echo bar >> $@',", + " tags = ['no-remote'],", + ")"); + + buildTarget("//:foobar"); + assertValidOutputFile("out/foo.txt", "foo\n"); + assertValidOutputFile("out/foobar.txt", "foo\nbar\n"); + + // Act: Modify the intermediate output and run a incremental build + var fooPath = getOutputPath("out/foo.txt"); + fooPath.delete(); + writeAbsolute(fooPath, "modified"); + + ActionEventCollector actionEventCollector = new ActionEventCollector(); + getRuntimeWrapper().registerSubscriber(actionEventCollector); + buildTarget("//:foobar"); + + // Assert: the stale intermediate file should be deleted by skyframe before executing the + // generating action. Since download minimal, the output didn't get downloaded. Since the input + // to action :foobar didn't change, we hit the skyframe cache, so the action node didn't event + // get evaluated. The input didn't get prefetched neither. + assertOutputDoesNotExist("out/foo.txt"); + assertValidOutputFile("out/foobar.txt", "foo\nbar\n"); + assertThat(actionEventCollector.getActionExecutedEvents()).hasSize(1); + assertThat(actionEventCollector.getCachedActionEvents()).isEmpty(); + var executedAction = actionEventCollector.getActionExecutedEvents().get(0).getAction(); + assertThat(executedAction.getPrimaryOutput().getFilename()).isEqualTo("foo.txt"); + } + protected void assertOutputsDoNotExist(String target) throws Exception { for (Artifact output : getArtifacts(target)) { assertWithMessage( @@ -705,4 +826,8 @@ protected void restartServer() throws Exception { // Simulates a server restart createRuntimeWrapper(); } + + protected static String lineSeparator() { + return System.getProperty("line.separator"); + } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index 487fe3404f1545..5a6f9830eb738f 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java @@ -15,6 +15,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; @@ -117,7 +118,7 @@ public void testGetInputStream() throws Exception { return Futures.immediateFuture(null); }) .when(inputFetcher) - .downloadFile(eq(remoteArtifact.getPath()), eq(inputs.getMetadata(remoteArtifact))); + .downloadFile(eq(remoteArtifact.getPath()), any(), eq(inputs.getMetadata(remoteArtifact))); // act Path remoteActionFsPath = actionFs.getPath(remoteArtifact.getPath().asFragment()); @@ -132,7 +133,7 @@ public void testGetInputStream() throws Exception { assertThat(actualRemoteContents).isEqualTo("remote contents"); assertThat(actualLocalContents).isEqualTo("local contents"); verify(inputFetcher) - .downloadFile(eq(remoteArtifact.getPath()), eq(inputs.getMetadata(remoteArtifact))); + .downloadFile(eq(remoteArtifact.getPath()), any(), eq(inputs.getMetadata(remoteArtifact))); verifyNoMoreInteractions(inputFetcher); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java index e82fcfede54211..e919392fda5d1b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java @@ -152,6 +152,21 @@ public void prefetchFiles_missingFiles_failsWithSpecificMessage() throws Excepti .contains(String.format("%s/%s", digest.getHash(), digest.getSizeBytes())); } + @Test + public void missingInputs_addedToList() { + Map metadata = new HashMap<>(); + Map cas = new HashMap<>(); + Artifact a = createRemoteArtifact("file", "hello world", metadata, /* cas= */ null); + MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); + AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); + + assertThrows( + ExecException.class, + () -> wait(prefetcher.prefetchFiles(metadata.keySet(), metadataProvider))); + + assertThat(prefetcher.getMissingActionInputs()).contains(a); + } + private RemoteCache newCache( RemoteOptions options, DigestUtil digestUtil, Map cas) { Map cacheEntries = Maps.newHashMapWithExpectedSize(cas.size()); diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/BUILD b/src/test/java/com/google/devtools/build/lib/remote/util/BUILD index ef1abf5595e292..d5d0bfaa71d13c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/util/BUILD @@ -55,6 +55,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", "//third_party:guava", + "//third_party:jsr305", "@bazel_tools//tools/java/runfiles", ], ) diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/IntegrationTestUtils.java b/src/test/java/com/google/devtools/build/lib/remote/util/IntegrationTestUtils.java index e2d2df95f93f8c..ded66dd44f2c62 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/IntegrationTestUtils.java +++ b/src/test/java/com/google/devtools/build/lib/remote/util/IntegrationTestUtils.java @@ -16,6 +16,7 @@ import static com.google.devtools.build.lib.testutil.TestUtils.tmpDirFile; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.shell.Subprocess; import com.google.devtools.build.lib.shell.SubprocessBuilder; @@ -34,6 +35,7 @@ import java.nio.file.Paths; import java.util.Comparator; import java.util.Random; +import javax.annotation.Nullable; /** Integration test utilities. */ public final class IntegrationTestUtils { @@ -113,20 +115,9 @@ public static WorkerInstance startWorker(boolean useHttp) PathFragment workPath = testTmpDir.getRelative("remote.work_path"); PathFragment casPath = testTmpDir.getRelative("remote.cas_path"); int workerPort = pickUnusedRandomPort(); - ensureMkdir(workPath); - ensureMkdir(casPath); - String workerPath = Runfiles.create().rlocation(WORKER_PATH.getSafePathString()); - Subprocess workerProcess = - new SubprocessBuilder() - .setArgv( - ImmutableList.of( - workerPath, - "--work_path=" + workPath.getSafePathString(), - "--cas_path=" + casPath.getSafePathString(), - (useHttp ? "--http_listen_port=" : "--listen_port=") + workerPort)) - .start(); - waitForPortOpen(workerProcess, workerPort); - return new WorkerInstance(workerProcess, workerPort, workPath, casPath); + var worker = new WorkerInstance(useHttp, workerPort, workPath, casPath); + worker.start(); + return worker; } private static void ensureMkdir(PathFragment path) throws IOException { @@ -140,25 +131,50 @@ private static void ensureMkdir(PathFragment path) throws IOException { } public static class WorkerInstance { - private final Subprocess process; + @Nullable private Subprocess process; + private final boolean useHttp; private final int port; private final PathFragment workPath; private final PathFragment casPath; - private WorkerInstance( - Subprocess process, int port, PathFragment workPath, PathFragment casPath) { - this.process = process; + private WorkerInstance(boolean useHttp, int port, PathFragment workPath, PathFragment casPath) { + this.useHttp = useHttp; this.port = port; this.workPath = workPath; this.casPath = casPath; } + private void start() throws IOException, InterruptedException { + Preconditions.checkState(process == null); + ensureMkdir(workPath); + ensureMkdir(casPath); + String workerPath = Runfiles.create().rlocation(WORKER_PATH.getSafePathString()); + process = + new SubprocessBuilder() + .setArgv( + ImmutableList.of( + workerPath, + "--work_path=" + workPath.getSafePathString(), + "--cas_path=" + casPath.getSafePathString(), + (useHttp ? "--http_listen_port=" : "--listen_port=") + port)) + .start(); + waitForPortOpen(process, port); + } + public void stop() throws IOException { + Preconditions.checkNotNull(process); process.destroyAndWait(); + process = null; + deleteDir(workPath); deleteDir(casPath); } + public void restart() throws IOException, InterruptedException { + stop(); + start(); + } + private static void deleteDir(PathFragment path) throws IOException { try (var stream = Files.walk(Paths.get(path.getSafePathString()))) { stream.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete); diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java index 54a6dd94d28667..c67577c54b92b7 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java @@ -23,6 +23,7 @@ import build.bazel.remote.execution.v2.FileNode; import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy; import build.bazel.remote.execution.v2.SymlinkNode; +import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.remote.RemoteCache; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.disk.DiskCacheClient; @@ -30,6 +31,7 @@ import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.protobuf.ByteString; import java.io.IOException; /** A {@link RemoteCache} backed by an {@link DiskCacheClient}. */ @@ -86,4 +88,16 @@ public DigestUtil getDigestUtil() { public RemoteOptions getRemoteOptions() { return options; } + + @Override + public ListenableFuture uploadBlob( + RemoteActionExecutionContext context, Digest digest, ByteString data) { + return uploadBlob(context, digest, data, /* force= */ true); + } + + @Override + public ListenableFuture uploadFile( + RemoteActionExecutionContext context, Digest digest, Path file) { + return uploadFile(context, digest, file, /* force= */ true); + } }