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 a053d76695ff22..05d8bc8c5c0bd3 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 @@ -46,6 +46,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.io.FileNotFoundException; import java.io.IOException; +import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; @@ -317,6 +318,7 @@ private CachedOutputMetadata( private static CachedOutputMetadata loadCachedOutputMetadata( Action action, ActionCache.Entry entry, MetadataHandler metadataHandler) { + Instant now = Instant.now(); ImmutableMap.Builder remoteFileMetadata = ImmutableMap.builder(); ImmutableMap.Builder mergedTreeMetadata = @@ -330,6 +332,17 @@ private static CachedOutputMetadata loadCachedOutputMetadata( continue; } + // If any child is not alive, discard the entire tree + if (cachedTreeMetadata.childValues().values().stream() + .anyMatch(metadata -> !metadata.isAlive(now))) { + continue; + } + + if (cachedTreeMetadata.archivedFileValue().isPresent() + && !cachedTreeMetadata.archivedFileValue().get().isAlive(now)) { + continue; + } + TreeArtifactValue localTreeMetadata; try { localTreeMetadata = metadataHandler.getTreeArtifactValue(parent); @@ -377,7 +390,7 @@ private static CachedOutputMetadata loadCachedOutputMetadata( mergedTreeMetadata.put(parent, merged.build()); } else { RemoteFileArtifactValue cachedMetadata = entry.getOutputFile(artifact); - if (cachedMetadata == null) { + if (cachedMetadata == null || !cachedMetadata.isAlive(now)) { continue; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index b44f9e9a3b8045..b7ca668d84d401 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -34,6 +34,7 @@ import com.google.devtools.build.skyframe.SkyValue; import java.io.ByteArrayInputStream; import java.io.IOException; +import java.time.Instant; import java.util.Arrays; import java.util.Objects; import java.util.Optional; @@ -542,27 +543,34 @@ public static class RemoteFileArtifactValue extends FileArtifactValue { protected final byte[] digest; protected final long size; protected final int locationIndex; + // The time when the remote file expires in milliseconds since epoch. negative value means the + // remote file will never expire. This field doesn't contribute to the equality of the metadata. + protected final long expireAtEpochMilli; - private RemoteFileArtifactValue(byte[] digest, long size, int locationIndex) { + private RemoteFileArtifactValue( + byte[] digest, long size, int locationIndex, long expireAtEpochMilli) { this.digest = Preconditions.checkNotNull(digest); this.size = size; this.locationIndex = locationIndex; + this.expireAtEpochMilli = expireAtEpochMilli; } - public static RemoteFileArtifactValue create(byte[] digest, long size, int locationIndex) { - return new RemoteFileArtifactValue(digest, size, locationIndex); + public static RemoteFileArtifactValue create( + byte[] digest, long size, int locationIndex, long expireAtEpochMilli) { + return new RemoteFileArtifactValue(digest, size, locationIndex, expireAtEpochMilli); } public static RemoteFileArtifactValue create( byte[] digest, long size, int locationIndex, + long expireAtEpochMilli, @Nullable PathFragment materializationExecPath) { if (materializationExecPath != null) { return new RemoteFileArtifactValueWithMaterializationPath( - digest, size, locationIndex, materializationExecPath); + digest, size, locationIndex, expireAtEpochMilli, materializationExecPath); } - return new RemoteFileArtifactValue(digest, size, locationIndex); + return new RemoteFileArtifactValue(digest, size, locationIndex, expireAtEpochMilli); } @Override @@ -613,6 +621,18 @@ public int getLocationIndex() { return locationIndex; } + public long getExpireAtEpochMilli() { + return expireAtEpochMilli; + } + + public boolean isAlive(Instant now) { + if (expireAtEpochMilli < 0) { + return true; + } + + return now.toEpochMilli() < expireAtEpochMilli; + } + @Override public boolean wasModifiedSinceDigest(Path path) { return false; @@ -629,6 +649,7 @@ public String toString() { .add("digest", bytesToString(digest)) .add("size", size) .add("locationIndex", locationIndex) + .add("expireAtEpochMilli", expireAtEpochMilli) .toString(); } } @@ -644,8 +665,12 @@ public static final class RemoteFileArtifactValueWithMaterializationPath private final PathFragment materializationExecPath; private RemoteFileArtifactValueWithMaterializationPath( - byte[] digest, long size, int locationIndex, PathFragment materializationExecPath) { - super(digest, size, locationIndex); + byte[] digest, + long size, + int locationIndex, + long expireAtEpochMilli, + PathFragment materializationExecPath) { + super(digest, size, locationIndex, expireAtEpochMilli); this.materializationExecPath = Preconditions.checkNotNull(materializationExecPath); } @@ -679,6 +704,7 @@ public String toString() { .add("digest", bytesToString(digest)) .add("size", size) .add("locationIndex", locationIndex) + .add("expireAtEpochMilli", expireAtEpochMilli) .add("materializationExecPath", materializationExecPath) .toString(); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java index 3809a4f7db92ec..8d36e0639c8f65 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java @@ -76,7 +76,7 @@ public class CompactPersistentActionCache implements ActionCache { private static final int NO_INPUT_DISCOVERY_COUNT = -1; - private static final int VERSION = 15; + private static final int VERSION = 16; private static final class ActionMap extends PersistentMap { private final Clock clock; @@ -466,6 +466,8 @@ private static void encodeRemoteMetadata( VarInt.putVarInt(value.getLocationIndex(), sink); + VarInt.putVarLong(value.getExpireAtEpochMilli(), sink); + Optional materializationExecPath = value.getMaterializationExecPath(); if (materializationExecPath.isPresent()) { VarInt.putVarInt(1, sink); @@ -476,10 +478,11 @@ private static void encodeRemoteMetadata( } private static final int MAX_REMOTE_METADATA_SIZE = - DigestUtils.ESTIMATED_SIZE - + VarInt.MAX_VARLONG_SIZE - + VarInt.MAX_VARINT_SIZE - + VarInt.MAX_VARINT_SIZE; + DigestUtils.ESTIMATED_SIZE // digest + + VarInt.MAX_VARLONG_SIZE // size + + VarInt.MAX_VARINT_SIZE // locationIndex + + VarInt.MAX_VARINT_SIZE // expireAtEpochMilli + + VarInt.MAX_VARINT_SIZE; // materializationExecPath private static RemoteFileArtifactValue decodeRemoteMetadata( StringIndexer indexer, ByteBuffer source) throws IOException { @@ -489,6 +492,8 @@ private static RemoteFileArtifactValue decodeRemoteMetadata( int locationIndex = VarInt.getVarInt(source); + long expireAtEpochMilli = VarInt.getVarLong(source); + PathFragment materializationExecPath = null; int numMaterializationExecPath = VarInt.getVarInt(source); if (numMaterializationExecPath > 0) { @@ -499,7 +504,7 @@ private static RemoteFileArtifactValue decodeRemoteMetadata( PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source))); } - return RemoteFileArtifactValue.create(digest, size, locationIndex, materializationExecPath); + return RemoteFileArtifactValue.create(digest, size, locationIndex, expireAtEpochMilli, materializationExecPath); } /** @return action data encoded as a byte[] array. */ 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 dfd21d7d79e602..8a61755cc6ff74 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 @@ -115,11 +115,12 @@ public void updateContext(MetadataInjector metadataInjector) { this.metadataInjector = metadataInjector; } - void injectRemoteFile(PathFragment path, byte[] digest, long size) throws IOException { + void injectRemoteFile(PathFragment path, byte[] digest, long size, long expireAtEpochMilli) + throws IOException { if (!isOutput(path)) { return; } - remoteOutputTree.injectRemoteFile(path, digest, size); + remoteOutputTree.injectRemoteFile(path, digest, size, expireAtEpochMilli); } void flush() throws IOException { @@ -206,6 +207,7 @@ private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact outpu metadata.getDigest(), metadata.getSize(), metadata.getLocationIndex(), + metadata.getExpireAtEpochMilli(), // Avoid a double indirection when the target is already materialized as a symlink. metadata.getMaterializationExecPath().orElse(targetPath.relativeTo(execRoot))); @@ -214,8 +216,8 @@ private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact outpu } private RemoteFileArtifactValue createRemoteMetadata(RemoteFileInfo remoteFile) { - return RemoteFileArtifactValue.create( - remoteFile.getFastDigest(), remoteFile.getSize(), /* locationIndex= */ 1); + return RemoteFileArtifactValue.create(remoteFile.getFastDigest(), + remoteFile.getSize(), /* locationIndex= */ 1, remoteFile.getExpireAtEpochMilli()); } @Override @@ -743,7 +745,8 @@ protected FileInfo newFile(Clock clock, PathFragment path) { return new RemoteFileInfo(clock); } - void injectRemoteFile(PathFragment path, byte[] digest, long size) throws IOException { + void injectRemoteFile(PathFragment path, byte[] digest, long size, long expireAtEpochMilli) + throws IOException { createDirectoryAndParents(path.getParentDirectory()); InMemoryContentInfo node = getOrCreateWritableInode(path); // If a node was already existed and is not a remote file node (i.e. directory or symlink node @@ -753,7 +756,7 @@ void injectRemoteFile(PathFragment path, byte[] digest, long size) throws IOExce } RemoteFileInfo remoteFileInfo = (RemoteFileInfo) node; - remoteFileInfo.set(digest, size); + remoteFileInfo.set(digest, size, expireAtEpochMilli); } @Nullable @@ -771,13 +774,16 @@ static class RemoteFileInfo extends FileInfo { private byte[] digest; private long size; + private long expireAtEpochMilli; + RemoteFileInfo(Clock clock) { super(clock); } - private void set(byte[] digest, long size) { + private void set(byte[] digest, long size, long expireAtEpochMilli) { this.digest = digest; this.size = size; + this.expireAtEpochMilli = expireAtEpochMilli; } @Override @@ -804,5 +810,9 @@ public byte[] getFastDigest() { public long getSize() { return size; } + + public long getExpireAtEpochMilli() { + return expireAtEpochMilli; + } } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index d97c62cd4f7ab1..d331a671e829fc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -128,6 +128,7 @@ import io.reactivex.rxjava3.disposables.Disposable; import io.reactivex.rxjava3.schedulers.Schedulers; import java.io.IOException; +import java.time.Instant; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -806,7 +807,7 @@ private void createSymlinks(Iterable symlinks) throws IOExcepti } } - private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata metadata) + private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata metadata, long expireAtEpochMilli) throws IOException { FileSystem actionFileSystem = action.getSpawnExecutionContext().getActionFileSystem(); checkState(actionFileSystem instanceof RemoteActionFileSystem); @@ -825,7 +826,8 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met remoteActionFileSystem.injectRemoteFile( file.path().asFragment(), DigestUtil.toBinaryDigest(file.digest()), - file.digest().getSizeBytes()); + file.digest().getSizeBytes(), + expireAtEpochMilli); } } @@ -833,7 +835,8 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met remoteActionFileSystem.injectRemoteFile( file.path().asFragment(), DigestUtil.toBinaryDigest(file.digest()), - file.digest().getSizeBytes()); + file.digest().getSizeBytes(), + expireAtEpochMilli); } } @@ -1162,7 +1165,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } } - injectRemoteArtifacts(action, metadata); + var expireAtEpochMilli = Instant.now().plus(remoteOptions.remoteCacheTtl).toEpochMilli(); + injectRemoteArtifacts(action, metadata, expireAtEpochMilli); try (SilentCloseable c = Profiler.instance().profile("Remote.downloadInMemoryOutput")) { if (inMemoryOutput != null) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java index 27da47a8fb2da0..f016f07a24afc4 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java @@ -13,11 +13,16 @@ // limitations under the License. package com.google.devtools.build.lib.remote.options; +import com.google.devtools.common.options.Converter; +import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; import com.google.devtools.common.options.OptionsBase; +import com.google.devtools.common.options.OptionsParsingException; +import java.time.Duration; import java.util.List; +import java.util.regex.Pattern; /** Options for remote execution and distributed caching that shared between Bazel and Blaze. */ public class CommonRemoteOptions extends OptionsBase { @@ -33,4 +38,38 @@ public class CommonRemoteOptions extends OptionsBase { + " the client to request certain artifacts that might be needed locally (e.g. IDE" + " support)") public List remoteDownloadRegex; + + @Option( + name = "experimental_remote_cache_ttl", + defaultValue = "3h", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.EXECUTION}, + converter = RemoteDurationConverter.class, + help = + "The guaranteed minimal TTL of blobs in the remote cache after their digests are recently" + + " referenced e.g. by an ActionResult or FindMissingBlobs. Bazel does several" + + " optimizations based on the blobs' TTL e.g. doesn't repeatedly call" + + " GetActionResult in an incremental build. The value should be set slightly less" + + " than the real TTL since there is a gap between when the server returns the" + + " digests and when Bazel receives them.") + public Duration remoteCacheTtl; + + /** Returns the specified duration. Assumes seconds if unitless. */ + public static class RemoteDurationConverter extends Converter.Contextless { + + private static final Pattern UNITLESS_REGEX = Pattern.compile("^[0-9]+$"); + + @Override + public Duration convert(String input) throws OptionsParsingException { + if (UNITLESS_REGEX.matcher(input).matches()) { + input += "s"; + } + return new Converters.DurationConverter().convert(input, /* conversionContext= */ null); + } + + @Override + public String getTypeDescription() { + return "An immutable length of time."; + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 879ba1dfdcfce9..fd382284f9b88d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -203,7 +203,7 @@ public final class RemoteOptions extends CommonRemoteOptions { defaultValue = "60s", documentationCategory = OptionDocumentationCategory.REMOTE, effectTags = {OptionEffectTag.UNKNOWN}, - converter = RemoteTimeoutConverter.class, + converter = RemoteDurationConverter.class, help = "The maximum amount of time to wait for remote execution and cache calls. For the REST" + " cache, this is both the connect and the read timeout. Following units can be" @@ -224,24 +224,6 @@ public final class RemoteOptions extends CommonRemoteOptions { + "When not set, it will default to \"${hostname}/${instance_name}\".") public String remoteBytestreamUriPrefix; - /** Returns the specified duration. Assumes seconds if unitless. */ - public static class RemoteTimeoutConverter extends Converter.Contextless { - private static final Pattern UNITLESS_REGEX = Pattern.compile("^[0-9]+$"); - - @Override - public Duration convert(String input) throws OptionsParsingException { - if (UNITLESS_REGEX.matcher(input).matches()) { - input += "s"; - } - return new Converters.DurationConverter().convert(input, /*conversionContext=*/ null); - } - - @Override - public String getTypeDescription() { - return "An immutable length of time."; - } - } - @Option( name = "remote_accept_cached", defaultValue = "true", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java index 50ab2f99b171f8..47ce7d8610eb39 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.FileStateType; import com.google.devtools.build.lib.concurrent.ExecutorUtil; import com.google.devtools.build.lib.concurrent.Sharder; @@ -58,6 +59,7 @@ import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.WalkableGraph; import java.io.IOException; +import java.time.Instant; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -228,6 +230,8 @@ public NavigableSet get() { } }); + var now = Instant.now(); + boolean interrupted; try (SilentCloseable c = Profiler.instance().profile("getDirtyActionValues.stat_files")) { for (List> shard : outputShards) { @@ -239,7 +243,8 @@ public NavigableSet get() { knownModifiedOutputFiles, sortedKnownModifiedOutputFiles, trustRemoteArtifacts, - modifiedOutputsReceiver) + modifiedOutputsReceiver, + now) : batchStatJob( dirtyKeys, shard, @@ -247,7 +252,8 @@ public NavigableSet get() { knownModifiedOutputFiles, sortedKnownModifiedOutputFiles, trustRemoteArtifacts, - modifiedOutputsReceiver); + modifiedOutputsReceiver, + now); executor.execute(job); } @@ -273,7 +279,8 @@ private Runnable batchStatJob( ImmutableSet knownModifiedOutputFiles, Supplier> sortedKnownModifiedOutputFiles, boolean trustRemoteArtifacts, - ModifiedOutputsReceiver modifiedOutputsReceiver) { + ModifiedOutputsReceiver modifiedOutputsReceiver, + Instant now) { return () -> { Map> fileToKeyAndValue = new HashMap<>(); Map> treeArtifactsToKeyAndValue = @@ -319,8 +326,8 @@ && shouldCheckFile(knownModifiedOutputFiles, artifact)) { try { stats = batchStatter.batchStat( - /*includeDigest=*/ true, - /*includeLinks=*/ true, + /* includeDigest= */ true, + /* includeLinks= */ true, Artifact.asPathFragments(artifacts)); } catch (IOException e) { logger.atWarning().withCause(e).log( @@ -331,7 +338,8 @@ && shouldCheckFile(knownModifiedOutputFiles, artifact)) { knownModifiedOutputFiles, sortedKnownModifiedOutputFiles, trustRemoteArtifacts, - modifiedOutputsReceiver) + modifiedOutputsReceiver, + now) .run(); return; } catch (InterruptedException e) { @@ -393,7 +401,8 @@ private Runnable outputStatJob( ImmutableSet knownModifiedOutputFiles, Supplier> sortedKnownModifiedOutputFiles, boolean trustRemoteArtifacts, - ModifiedOutputsReceiver modifiedOutputsReceiver) { + ModifiedOutputsReceiver modifiedOutputsReceiver, + Instant now) { return new Runnable() { @Override public void run() { @@ -405,7 +414,8 @@ public void run() { knownModifiedOutputFiles, sortedKnownModifiedOutputFiles, trustRemoteArtifacts, - modifiedOutputsReceiver)) { + modifiedOutputsReceiver, + now)) { dirtyKeys.add(keyAndValue.getFirst()); } } @@ -441,7 +451,8 @@ private boolean artifactIsDirtyWithDirectSystemCalls( ImmutableSet knownModifiedOutputFiles, boolean trustRemoteArtifacts, Map.Entry entry, - ModifiedOutputsReceiver modifiedOutputsReceiver) { + ModifiedOutputsReceiver modifiedOutputsReceiver, + Instant now) { Artifact file = entry.getKey(); FileArtifactValue lastKnownData = entry.getValue(); if (file.isMiddlemanArtifact() || !shouldCheckFile(knownModifiedOutputFiles, file)) { @@ -453,7 +464,8 @@ private boolean artifactIsDirtyWithDirectSystemCalls( boolean trustRemoteValue = fileMetadata.getType() == FileStateType.NONEXISTENT && lastKnownData.isRemote() - && trustRemoteArtifacts; + && trustRemoteArtifacts + && ((RemoteFileArtifactValue) lastKnownData).isAlive(now); if (!trustRemoteValue && fileMetadata.couldBeModifiedSince(lastKnownData)) { modifiedOutputsReceiver.reportModifiedOutputFile( fileMetadata.getType() != FileStateType.NONEXISTENT @@ -475,11 +487,12 @@ private boolean actionValueIsDirtyWithDirectSystemCalls( ImmutableSet knownModifiedOutputFiles, Supplier> sortedKnownModifiedOutputFiles, boolean trustRemoteArtifacts, - ModifiedOutputsReceiver modifiedOutputsReceiver) { + ModifiedOutputsReceiver modifiedOutputsReceiver, + Instant now) { boolean isDirty = false; for (Map.Entry entry : actionValue.getAllFileValues().entrySet()) { if (artifactIsDirtyWithDirectSystemCalls( - knownModifiedOutputFiles, trustRemoteArtifacts, entry, modifiedOutputsReceiver)) { + knownModifiedOutputFiles, trustRemoteArtifacts, entry, modifiedOutputsReceiver, now)) { isDirty = true; } } @@ -488,31 +501,31 @@ private boolean actionValueIsDirtyWithDirectSystemCalls( actionValue.getAllTreeArtifactValues().entrySet()) { TreeArtifactValue tree = entry.getValue(); - if (!tree.isEntirelyRemote()) { - for (Map.Entry childEntry : - tree.getChildValues().entrySet()) { - if (artifactIsDirtyWithDirectSystemCalls( - knownModifiedOutputFiles, - trustRemoteArtifacts, - childEntry, - modifiedOutputsReceiver)) { - isDirty = true; - } + for (Map.Entry childEntry : + tree.getChildValues().entrySet()) { + if (artifactIsDirtyWithDirectSystemCalls( + knownModifiedOutputFiles, + trustRemoteArtifacts, + childEntry, + modifiedOutputsReceiver, + now)) { + isDirty = true; } - isDirty = - isDirty - || tree.getArchivedRepresentation() - .map( - archivedRepresentation -> - artifactIsDirtyWithDirectSystemCalls( - knownModifiedOutputFiles, - trustRemoteArtifacts, - Maps.immutableEntry( - archivedRepresentation.archivedTreeFileArtifact(), - archivedRepresentation.archivedFileValue()), - modifiedOutputsReceiver)) - .orElse(false); } + isDirty = + isDirty + || tree.getArchivedRepresentation() + .map( + archivedRepresentation -> + artifactIsDirtyWithDirectSystemCalls( + knownModifiedOutputFiles, + trustRemoteArtifacts, + Maps.immutableEntry( + archivedRepresentation.archivedTreeFileArtifact(), + archivedRepresentation.archivedFileValue()), + modifiedOutputsReceiver, + now)) + .orElse(false); Artifact treeArtifact = entry.getKey(); if (shouldCheckTreeArtifact(sortedKnownModifiedOutputFiles.get(), treeArtifact) diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index 7797cf67ccdd40..485bc3cc284bb9 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -463,7 +463,13 @@ private RemoteFileArtifactValue createRemoteFileMetadata(String content) { private RemoteFileArtifactValue createRemoteFileMetadata( String content, @Nullable PathFragment materializationExecPath) { byte[] bytes = content.getBytes(UTF_8); - return RemoteFileArtifactValue.create(digest(bytes), bytes.length, 1, materializationExecPath); + return RemoteFileArtifactValue.create(digest(bytes), bytes.length, 1, /* expireAtEpochMilli= */ -1, materializationExecPath); + } + + private RemoteFileArtifactValue createRemoteFileMetadata( + String content, long expireAtEpochMilli, @Nullable PathFragment materializationExecPath) { + byte[] bytes = content.getBytes(UTF_8); + return RemoteFileArtifactValue.create(digest(bytes), bytes.length, 1, expireAtEpochMilli, materializationExecPath); } private static TreeArtifactValue createTreeMetadata( @@ -588,6 +594,33 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { assertThat(metadataHandler.getMetadata(output)).isEqualTo(createRemoteFileMetadata(content)); } + @Test + public void saveOutputMetadata_remoteFileExpired_remoteFileMetadataNotLoaded() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + String content = "content"; + Action action = new InjectOutputFileMetadataAction(output, createRemoteFileMetadata(content, /* expireAtEpochMilli= */ 0, /* materializationExecPath= */ null)); + MetadataHandler metadataHandler = new FakeMetadataHandler(); + + runAction(action); + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /* resolvedCacheArtifacts= */ null, + /* clientEnv= */ ImmutableMap.of(), + OutputPermissions.READONLY, + /* handler= */ null, + metadataHandler, + /* artifactExpander= */ null, + /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), + /* loadCachedOutputMetadata= */ true); + + assertThat(output.getPath().exists()).isFalse(); + assertThat(token).isNotNull(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNull(); + } + @Test public void saveOutputMetadata_remoteOutputUnavailable_remoteFileMetadataNotLoaded() throws Exception { @@ -999,6 +1032,44 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws assertThat(metadataHandler.getTreeArtifactValue(output)).isEqualTo(expectedMetadata); } + @Test + public void saveOutputMetadata_treeFileExpired_treeMetadataNotLoaded() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + ImmutableMap children = + ImmutableMap.of( + "file1", createRemoteFileMetadata("content1"), + "file2", createRemoteFileMetadata("content2", /* expireAtEpochMilli= */ 0, /* materializationExecPath= */ null)); + Action action = + new InjectOutputTreeMetadataAction( + output, + createTreeMetadata( + output, + children, + /* archivedArtifactValue= */ Optional.empty(), + /* materializationExecPath= */ Optional.empty())); + MetadataHandler metadataHandler = new FakeMetadataHandler(); + + runAction(action); + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /* resolvedCacheArtifacts= */ null, + /* clientEnv= */ ImmutableMap.of(), + OutputPermissions.READONLY, + /* handler= */ null, + metadataHandler, + /* artifactExpander= */ null, + /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), + /* loadCachedOutputMetadata= */ true); + + assertThat(output.getPath().exists()).isFalse(); + assertThat(token).isNotNull(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNull(); + } + private static void writeContentAsLatin1(Path path, String content) throws IOException { Path parent = path.getParentDirectory(); if (parent != null) { diff --git a/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java b/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java index 764adbf976a4b2..24c8c1ec231a0e 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java @@ -40,7 +40,7 @@ @RunWith(JUnit4.class) public class CompletionContextTest { private static final FileArtifactValue DUMMY_METADATA = - RemoteFileArtifactValue.create(/*digest=*/ new byte[0], /*size=*/ 0, /*locationIndex=*/ 0); + RemoteFileArtifactValue.create(/*digest=*/ new byte[0], /*size=*/ 0, /*locationIndex=*/ 0, /* expireAtEpochMilli= */ -1); private Path execRoot; private ArtifactRoot outputRoot; diff --git a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java index c9f6d59a241452..aad3303bfb9c24 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java @@ -35,6 +35,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.time.Instant; import java.util.Map; import java.util.Optional; import javax.annotation.Nullable; @@ -198,7 +199,10 @@ private FileArtifactValue createLocalMetadata(Artifact artifact, String content) } private RemoteFileArtifactValue createRemoteMetadata( - Artifact artifact, String content, @Nullable PathFragment materializationExecPath) { + Artifact artifact, + String content, + long expireAtEpochMilli, + @Nullable PathFragment materializationExecPath) { byte[] bytes = content.getBytes(StandardCharsets.UTF_8); byte[] digest = artifact @@ -208,7 +212,14 @@ private RemoteFileArtifactValue createRemoteMetadata( .getHashFunction() .hashBytes(bytes) .asBytes(); - return RemoteFileArtifactValue.create(digest, bytes.length, 1, materializationExecPath); + return RemoteFileArtifactValue.create( + digest, bytes.length, 1, expireAtEpochMilli, materializationExecPath); + } + + private RemoteFileArtifactValue createRemoteMetadata( + Artifact artifact, String content, @Nullable PathFragment materializationExecPath) { + return createRemoteMetadata( + artifact, content, /* expireAtEpochMilli= */ -1, materializationExecPath); } private RemoteFileArtifactValue createRemoteMetadata(Artifact artifact, String content) { @@ -252,6 +263,24 @@ public void putAndGet_savesRemoteFileMetadata() { assertThat(entry.getOutputFile(artifact)).isEqualTo(metadata); } + @Test + public void putAndGet_savesRemoteFileMetadata_withExpireAtEpochMilli() { + String key = "key"; + ActionCache.Entry entry = + new ActionCache.Entry(key, ImmutableMap.of(), false, OutputPermissions.READONLY); + Artifact artifact = ActionsTestUtil.DUMMY_ARTIFACT; + long expireAtEpochMilli = Instant.now().toEpochMilli(); + RemoteFileArtifactValue metadata = + createRemoteMetadata( + artifact, "content", expireAtEpochMilli, /* materializationExecPath= */ null); + entry.addOutputFile(artifact, metadata, /*saveFileMetadata=*/ true); + + cache.put(key, entry); + entry = cache.get(key); + + assertThat(entry.getOutputFile(artifact).getExpireAtEpochMilli()).isEqualTo(expireAtEpochMilli); + } + @Test public void putAndGet_savesRemoteFileMetadata_withmaterializationExecPath() { String key = "key"; 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 8c097089a78200..c4c99508fb7b06 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 @@ -104,6 +104,7 @@ protected Artifact createRemoteArtifact( hashCode.asBytes(), contentsBytes.length, /* locationIndex= */ 1, + /* expireAtEpochMilli= */ -1, materializationExecPath); metadata.put(a, f); if (cas != null) { @@ -154,7 +155,10 @@ protected Pair> createRemoteTre HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contents); RemoteFileArtifactValue childValue = RemoteFileArtifactValue.create( - hashCode.asBytes(), contents.length, /* locationIndex= */ 1); + hashCode.asBytes(), + contents.length, + /* locationIndex= */ 1, + /* expireAtEpochMilli= */ -1); treeBuilder.putChild(child, childValue); metadata.put(child, childValue); cas.put(hashCode, contents); 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 c1d973e489fe33..e7d982133b3d11 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 @@ -944,6 +944,97 @@ public void remoteCacheEvictBlobs_whenPrefetchingInputTree_incrementalBuildCanCo "a/bar.out", "file-inside\nupdated bar" + lineSeparator(), /* isLocal= */ true); } + @Test + public void remoteFilesExpiredBetweenBuilds_rerunGeneratingActions() 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) > $@',", + ")"); + 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 + setDownloadToplevel(); + addOptions("--experimental_remote_cache_ttl=1s"); + buildTarget("//a:bar"); + assertOutputDoesNotExist("a/foo.out"); + + // Evict blobs from remote cache + evictAllBlobs(); + + // Act: Do an incremental build + write("a/bar.in", "updated bar"); + buildTarget("//a:bar"); + + waitDownloads(); + + // Assert: target was successfully built + assertValidOutputFile("a/bar.out", "foo" + lineSeparator() + "updated bar" + lineSeparator()); + } + + @Test + public void remoteTreeFilesExpiredBetweenBuilds_rerunGeneratingActions() throws Exception { + // Arrange: Prepare workspace and populate remote cache + write("BUILD"); + writeOutputDirRule(); + write( + "a/BUILD", + "load('//:output_dir.bzl', 'output_dir')", + "output_dir(", + " name = 'foo.out',", + " content_map = {'file-inside': 'hello world'},", + ")", + "genrule(", + " name = 'bar',", + " srcs = ['foo.out', 'bar.in'],", + " outs = ['bar.out'],", + " cmd = '( ls $(location :foo.out); cat $(location :bar.in) ) > $@',", + ")"); + write("a/bar.in", "bar"); + + // Populate remote cache + buildTarget("//a:bar"); + getOutputPath("a/foo.out").deleteTreesBelow(); + getOutputPath("a/bar.out").delete(); + getOutputBase().getRelative("action_cache").deleteTreesBelow(); + restartServer(); + + // Clean build, foo.out isn't downloaded + setDownloadToplevel(); + addOptions("--experimental_remote_cache_ttl=1s"); + buildTarget("//a:bar"); + assertOutputDoesNotExist("a/foo.out/file-inside"); + + // Evict blobs from remote cache + evictAllBlobs(); + + // Act: Do an incremental build + write("a/bar.in", "updated bar"); + buildTarget("//a:bar"); + + // Assert: target was successfully built + assertValidOutputFile( + "a/bar.out", "file-inside\nupdated bar" + lineSeparator(), /* isLocal= */ true); + } + protected void assertOutputsDoNotExist(String target) throws Exception { for (Artifact output : getArtifacts(target)) { assertWithMessage( 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 9b33e0778a2a3d..9e1eb36126e601 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 @@ -326,7 +326,8 @@ protected void injectRemoteFile(FileSystem actionFs, PathFragment path, String c byte[] contentBytes = content.getBytes(StandardCharsets.UTF_8); HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contentBytes); ((RemoteActionFileSystem) actionFs) - .injectRemoteFile(path, hashCode.asBytes(), contentBytes.length); + .injectRemoteFile( + path, hashCode.asBytes(), contentBytes.length, /* expireAtEpochMilli= */ -1); } @Override @@ -342,7 +343,8 @@ private Artifact createRemoteArtifact( byte[] b = contents.getBytes(StandardCharsets.UTF_8); HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b); RemoteFileArtifactValue f = - RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1); + RemoteFileArtifactValue.create( + h.asBytes(), b.length, /* locationIndex= */ 1, /* expireAtEpochMilli= */ -1); inputs.putWithNoDepOwner(a, f); return a; } @@ -364,7 +366,8 @@ private TreeArtifactValue createRemoteTreeArtifactValue( byte[] b = entry.getValue().getBytes(StandardCharsets.UTF_8); HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b); RemoteFileArtifactValue childMeta = - RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 0); + RemoteFileArtifactValue.create( + h.asBytes(), b.length, /* locationIndex= */ 0, /* expireAtEpochMilli= */ -1); builder.putChild(child, childMeta); } return builder.build(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 72cffa1aa77d85..ec1cf0a3acc38f 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -965,7 +965,8 @@ public void downloadOutputs_outputFilesWithoutTopLevel_inject() throws Exception .injectRemoteFile( eq(execRoot.asFragment().getRelative(a1.getExecPath())), eq(toBinaryDigest(d1)), - eq(d1.getSizeBytes())); + eq(d1.getSizeBytes()), + any()); Path outputBase = checkNotNull(artifactRoot.getRoot().asPath()); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); assertThat(context.isLockOutputFilesCalled()).isTrue(); @@ -1005,12 +1006,14 @@ public void downloadOutputs_outputFilesWithMinimal_injectingMetadata() throws Ex .injectRemoteFile( eq(execRoot.asFragment().getRelative(a1.getExecPath())), eq(toBinaryDigest(d1)), - eq(d1.getSizeBytes())); + eq(d1.getSizeBytes()), + any()); verify(actionFileSystem) .injectRemoteFile( eq(execRoot.asFragment().getRelative(a2.getExecPath())), eq(toBinaryDigest(d2)), - eq(d2.getSizeBytes())); + eq(d2.getSizeBytes()), + any()); Path outputBase = checkNotNull(artifactRoot.getRoot().asPath()); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); assertThat(context.isLockOutputFilesCalled()).isTrue(); @@ -1063,12 +1066,14 @@ public void downloadOutputs_outputDirectoriesWithMinimal_injectingMetadata() thr .injectRemoteFile( eq(execRoot.asFragment().getRelative("outputs/dir/file1")), eq(toBinaryDigest(d1)), - eq(d1.getSizeBytes())); + eq(d1.getSizeBytes()), + any()); verify(actionFileSystem) .injectRemoteFile( eq(execRoot.asFragment().getRelative("outputs/dir/a/file2")), eq(toBinaryDigest(d2)), - eq(d2.getSizeBytes())); + eq(d2.getSizeBytes()), + any()); Path outputBase = checkNotNull(artifactRoot.getRoot().asPath()); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); assertThat(context.isLockOutputFilesCalled()).isTrue(); @@ -1201,12 +1206,14 @@ public void downloadOutputs_inMemoryOutputWithMinimal_downloadIt() throws Except .injectRemoteFile( eq(execRoot.asFragment().getRelative(a1.getExecPath())), eq(toBinaryDigest(d1)), - eq(d1.getSizeBytes())); + eq(d1.getSizeBytes()), + any()); verify(actionFileSystem) .injectRemoteFile( eq(execRoot.asFragment().getRelative(a2.getExecPath())), eq(toBinaryDigest(d2)), - eq(d2.getSizeBytes())); + eq(d2.getSizeBytes()), + any()); Path outputBase = checkNotNull(artifactRoot.getRoot().asPath()); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); assertThat(context.isLockOutputFilesCalled()).isTrue(); @@ -1254,7 +1261,8 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw .injectRemoteFile( eq(execRoot.asFragment().getRelative(a1.getExecPath())), eq(toBinaryDigest(d1)), - eq(d1.getSizeBytes())); + eq(d1.getSizeBytes()), + any()); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/remote/options/RemoteOptionsTest.java b/src/test/java/com/google/devtools/build/lib/remote/options/RemoteOptionsTest.java index e6425a4889f650..90c09dfa6895a5 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/options/RemoteOptionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/options/RemoteOptionsTest.java @@ -79,7 +79,7 @@ public void testRemoteTimeoutOptionsConverterWithoutUnit() { try { int seconds = 60; Duration convert = - new RemoteOptions.RemoteTimeoutConverter().convert(String.valueOf(seconds)); + new CommonRemoteOptions.RemoteDurationConverter().convert(String.valueOf(seconds)); assertThat(Duration.ofSeconds(seconds)).isEqualTo(convert); } catch (OptionsParsingException e) { fail(e.getMessage()); @@ -90,7 +90,8 @@ public void testRemoteTimeoutOptionsConverterWithoutUnit() { public void testRemoteTimeoutOptionsConverterWithUnit() { try { int milliseconds = 60; - Duration convert = new RemoteOptions.RemoteTimeoutConverter().convert(milliseconds + "ms"); + Duration convert = + new CommonRemoteOptions.RemoteDurationConverter().convert(milliseconds + "ms"); assertThat(Duration.ofMillis(milliseconds)).isEqualTo(convert); } catch (OptionsParsingException e) { fail(e.getMessage()); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java index 07dfdbed185b93..f81bb23bc17ce6 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java @@ -43,10 +43,16 @@ public class ActionExecutionValueTest { private static final FileArtifactValue VALUE_1 = RemoteFileArtifactValue.create( - /* digest= */ new byte[0], /* size= */ 0, /* locationIndex= */ 1); + /* digest= */ new byte[0], + /* size= */ 0, + /* locationIndex= */ 1, + /* expireAtEpochMilli= */ -1); private static final FileArtifactValue VALUE_2 = RemoteFileArtifactValue.create( - /* digest= */ new byte[0], /* size= */ 0, /* locationIndex= */ 2); + /* digest= */ new byte[0], + /* size= */ 0, + /* locationIndex= */ 2, + /* expireAtEpochMilli= */ -1); private static final ActionLookupKey KEY = mock(ActionLookupKey.class); private static final ActionLookupData ACTION_LOOKUP_DATA_1 = ActionLookupData.create(KEY, 1); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java index 411c9dc44b780a..3ac63434af6047 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java @@ -307,7 +307,9 @@ public void resettingOutputs() throws Exception { assertThat(chmodCalls).containsExactly(outputPath, 0555); // Inject a remote file of size 42. - handler.injectFile(artifact, RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 42, 0)); + handler.injectFile( + artifact, + RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 42, 0, /* expireAtEpochMilli= */ -1)); assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(42); // Reset this output, which will make the handler stat the file again. @@ -331,7 +333,9 @@ public void injectRemoteArtifactMetadata() throws Exception { byte[] digest = new byte[] {1, 2, 3}; int size = 10; handler.injectFile( - artifact, RemoteFileArtifactValue.create(digest, size, /* locationIndex= */ 1)); + artifact, + RemoteFileArtifactValue.create( + digest, size, /* locationIndex= */ 1, /* expireAtEpochMilli= */ -1)); FileArtifactValue v = handler.getMetadata(artifact); assertThat(v).isNotNull(); @@ -354,7 +358,8 @@ public void cannotInjectTreeArtifactChildIndividually() { /* outputs= */ ImmutableSet.of(treeArtifact)); handler.prepareForActionExecution(); - RemoteFileArtifactValue childValue = RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1); + RemoteFileArtifactValue childValue = + RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1); assertThrows(IllegalArgumentException.class, () -> handler.injectFile(child, childValue)); assertThat(handler.getOutputStore().getAllArtifactData()).isEmpty(); @@ -378,7 +383,8 @@ public void canInjectTemplateExpansionOutput() { /* outputs= */ ImmutableSet.of(treeArtifact)); handler.prepareForActionExecution(); - RemoteFileArtifactValue value = RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1); + RemoteFileArtifactValue value = + RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1); handler.injectFile(output, value); assertThat(handler.getOutputStore().getAllArtifactData()).containsExactly(output, value); @@ -402,10 +408,12 @@ public void injectRemoteTreeArtifactMetadata() throws Exception { TreeArtifactValue.newBuilder(treeArtifact) .putChild( TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), - RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1)) + RemoteFileArtifactValue.create( + new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1)) .putChild( TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), - RemoteFileArtifactValue.create(new byte[] {4, 5, 6}, 10, 1)) + RemoteFileArtifactValue.create( + new byte[] {4, 5, 6}, 10, 1, /* expireAtEpochMilli= */ -1)) .build(); handler.injectTree(treeArtifact, tree); @@ -638,7 +646,8 @@ public void transformAfterInputDiscovery() throws Exception { assertThat(handler.getMetadata(unknown)).isNull(); OutputStore newStore = new OutputStore(); - FileArtifactValue knownMetadata = RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1); + FileArtifactValue knownMetadata = + RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1); newStore.putArtifactData(known, knownMetadata); ActionMetadataHandler newHandler = handler.transformAfterInputDiscovery(newStore); assertThat(newHandler.getOutputStore()).isNotEqualTo(handler.getOutputStore()); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java index 0435a19691606f..6ad9633d6b4487 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java @@ -20,6 +20,7 @@ import com.google.common.io.BaseEncoding; import com.google.common.testing.EqualsTester; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -78,6 +79,10 @@ public void testEqualsAndHashCode() { .addEqualityGroup( FileArtifactValue.createForDirectoryWithMtime(2), FileArtifactValue.createForDirectoryWithMtime(2)) + .addEqualityGroup( + // expireAtEpochMilli doesn't contribute to the equality + RemoteFileArtifactValue.create(toBytes("00112233445566778899AABBCCDDEEFF"), 1, 1, 1), + RemoteFileArtifactValue.create(toBytes("00112233445566778899AABBCCDDEEFF"), 1, 1, 2)) .addEqualityGroup(FileArtifactValue.OMITTED_FILE_MARKER) .addEqualityGroup(FileArtifactValue.MISSING_FILE_MARKER) .addEqualityGroup(FileArtifactValue.DEFAULT_MIDDLEMAN) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java index 3c2ba720e885a2..f0c7200ac11c16 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java @@ -1324,10 +1324,15 @@ private static ActionExecutionValue actionValueWithRemoteArtifact( } private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents) { + return createRemoteFileArtifactValue(contents, /* expireAtEpochMilli= */ -1); + } + + private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents, long expireAtEpochMilli) { byte[] data = contents.getBytes(); DigestHashFunction hashFn = fs.getDigestFunction(); HashCode hash = hashFn.getHashFunction().hashBytes(data); - return RemoteFileArtifactValue.create(hash.asBytes(), data.length, -1); + return RemoteFileArtifactValue.create( + hash.asBytes(), data.length, -1, expireAtEpochMilli); } @Test @@ -1384,6 +1389,45 @@ public void testRemoteAndLocalArtifacts() throws Exception { .containsExactly(actionKey1); } + @Test + public void testRemoteArtifactsExpired() throws Exception { + // Test that if injected remote artifacts expired, they are considered as dirty. + SkyKey actionKey1 = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); + SkyKey actionKey2 = ActionLookupData.create(ACTION_LOOKUP_KEY, 1); + + Artifact out1 = createDerivedArtifact("foo"); + Artifact out2 = createDerivedArtifact("bar"); + Map metadataToInject = new HashMap<>(); + metadataToInject.put( + actionKey1, + actionValueWithRemoteArtifact(out1, createRemoteFileArtifactValue("foo-content"))); + metadataToInject.put( + actionKey2, + actionValueWithRemoteArtifact(out2, createRemoteFileArtifactValue("bar-content", /* expireAtEpochMilli= */ 0))); + differencer.inject(metadataToInject); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setParallelism(1) + .setEventHandler(NullEventHandler.INSTANCE) + .build(); + assertThat( + evaluator + .evaluate(ImmutableList.of(actionKey1, actionKey2), evaluationContext) + .hasError()) + .isFalse(); + assertThat( + new FilesystemValueChecker(/*tsgm=*/ null, SyscallCache.NO_CACHE, FSVC_THREADS_FOR_TEST) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED, + /* trustRemoteArtifacts= */ true, + (ignored, ignored2) -> {})) + .containsExactly(actionKey2); + } + @Test public void testRemoteAndLocalTreeArtifacts() throws Exception { // Test that injected remote tree artifacts are trusted by the FileSystemValueChecker @@ -1418,7 +1462,7 @@ public void testRemoteAndLocalTreeArtifacts() throws Exception { evaluator.getValues(), /* batchStatter= */ null, ModifiedFileSet.EVERYTHING_MODIFIED, - /* trustRemoteArtifacts= */ false, + /* trustRemoteArtifacts= */ true, (ignored, ignored2) -> {})) .isEmpty(); @@ -1431,11 +1475,49 @@ public void testRemoteAndLocalTreeArtifacts() throws Exception { evaluator.getValues(), /* batchStatter= */ null, ModifiedFileSet.EVERYTHING_MODIFIED, - /* trustRemoteArtifacts= */ false, + /* trustRemoteArtifacts= */ true, (ignored, ignored2) -> {})) .containsExactly(actionKey); } + @Test + public void testRemoteTreeArtifactsExpired() throws Exception { + // Test that if injected remote tree artifacts are expired, they are considered as dirty. + SkyKey actionKey = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); + + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + treeArtifact.getPath().createDirectoryAndParents(); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(treeArtifact) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), + createRemoteFileArtifactValue("foo-content")) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), + createRemoteFileArtifactValue("bar-content", /* expireAtEpochMilli= */ 0)) + .build(); + + differencer.inject(ImmutableMap.of(actionKey, actionValueWithTreeArtifact(treeArtifact, tree))); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setParallelism(1) + .setEventHandler(NullEventHandler.INSTANCE) + .build(); + assertThat(evaluator.evaluate(ImmutableList.of(actionKey), evaluationContext).hasError()) + .isFalse(); + assertThat( + new FilesystemValueChecker(/*tsgm=*/ null, SyscallCache.NO_CACHE, FSVC_THREADS_FOR_TEST) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED, + /* trustRemoteArtifacts= */ true, + (ignored, ignored2) -> {})) + .containsExactly(actionKey); + } + @Test public void testPropagatesRuntimeExceptions() throws Exception { Collection values = diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java index 62a8c848aa7a6f..d822d1abbc1b39 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java @@ -616,10 +616,16 @@ public void remoteDirectoryInjection() throws Exception { SpecialArtifact out = createTreeArtifact("output"); RemoteFileArtifactValue remoteFile1 = RemoteFileArtifactValue.create( - Hashing.sha256().hashString("one", UTF_8).asBytes(), /*size=*/ 3, /*locationIndex=*/ 1); + Hashing.sha256().hashString("one", UTF_8).asBytes(), + /*size=*/ 3, + /*locationIndex=*/ 1, + /* expireAtEpochMilli= */ -1); RemoteFileArtifactValue remoteFile2 = RemoteFileArtifactValue.create( - Hashing.sha256().hashString("two", UTF_8).asBytes(), /*size=*/ 3, /*locationIndex=*/ 2); + Hashing.sha256().hashString("two", UTF_8).asBytes(), + /*size=*/ 3, + /*locationIndex=*/ 2, + /* expireAtEpochMilli= */ -1); Action action = new SimpleTestAction(out) { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java index 663f3784b81f52..2ff5a90ad3aab1 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java @@ -711,7 +711,7 @@ private static SpecialArtifact createTreeArtifact(String execPath, ArtifactRoot } private static FileArtifactValue metadataWithId(int id) { - return RemoteFileArtifactValue.create(new byte[] {(byte) id}, id, id); + return RemoteFileArtifactValue.create(new byte[] {(byte) id}, id, id, /* expireAtEpochMilli= */ -1); } private static FileArtifactValue metadataWithIdNoDigest(int id) {