diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java index f714e9c543feac..94c28616927025 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java @@ -14,9 +14,9 @@ package com.google.devtools.build.lib.remote.disk; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static java.util.Comparator.comparing; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ComparisonChain; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.concurrent.AbstractQueueVisitor; import com.google.devtools.build.lib.concurrent.ErrorClassifier; @@ -29,6 +29,7 @@ import java.time.Duration; import java.time.Instant; import java.util.ArrayList; +import java.util.Comparator; import java.util.List; import java.util.Optional; import java.util.concurrent.ExecutorService; @@ -62,13 +63,23 @@ private record Entry(String path, long size, long mtime) {} */ public record CollectionPolicy(Optional maxSizeBytes, Optional maxAge) { + // Sort older entries before newer ones, tie breaking by path. This causes AC entries to be + // sorted before CAS entries with the same age, making it less likely for garbage collection + // to break referential integrity in the event that mtime resolution is insufficient. + private static final Comparator COMPARATOR = + (x, y) -> + ComparisonChain.start() + .compare(x.mtime(), y.mtime()) + .compare(x.path(), y.path()) + .result(); + /** * Returns the entries to be deleted. * * @param entries the full list of entries */ List getEntriesToDelete(List entries) { - entries.sort(comparing(Entry::mtime)); + entries.sort(COMPARATOR); long excessSizeBytes = getExcessSizeBytes(entries); long timeCutoff = getTimeCutoff(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java index 9e3fc3648bac6a..3701e8a22d4815 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java @@ -79,6 +79,21 @@ public void sizePolicy_collectsOldest() throws Exception { assertFilesDoNotExist("ac/abc", "cas/def"); } + @Test + public void sizePolicy_tieBreakByPath() throws Exception { + writeFiles( + Entry.of("ac/123", kbytes(1), daysAgo(1)), + Entry.of("cas/456", kbytes(1), daysAgo(1)), + Entry.of("ac/abc", kbytes(1), daysAgo(1)), + Entry.of("cas/def", kbytes(1), daysAgo(1))); + + CollectionStats stats = runGarbageCollector(Optional.of(kbytes(2)), Optional.empty()); + + assertThat(stats).isEqualTo(new CollectionStats(4, kbytes(4), 2, kbytes(2))); + assertFilesExist("cas/456", "cas/def"); + assertFilesDoNotExist("ac/123", "ac/abc"); + } + @Test public void agePolicy_noCollection() throws Exception { writeFiles(