Fix comparator used in ConcurrentArtifactPathTrie

The default comparator for `PathFragment` may sort unrelated paths between a path and a child path, which results in `contains` behaving incorrectly as soon as paths such as `foo/bar` and `foo/bar-baz` are present in the set. This is a bug introduced by #26639.

This is fixed by introducing a new `Comparator` for `PathFragment` that sorts `/` lower than any other character. This will also be used in future work on #21378 and is thus placed in `PathFragment`. It may become the default in the future, but not until Blaze has been confirmed to not rely on the order.

Closes #26714.

PiperOrigin-RevId: 792519419
Change-Id: I8bf8fc6b15cc5bcf0d303057a828fbd644032bab

Author: fmeum

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

@@ -368,7 +368,8 @@ public void maybeInvalidateSkyframeValues(MemoizingEvaluator memoizingEvaluator)
    */
   private static final class ConcurrentArtifactPathTrie {
     // Invariant: no path in this set is a prefix of another path.
-    private final ConcurrentSkipListSet<PathFragment> paths = new ConcurrentSkipListSet<>();
+    private final ConcurrentSkipListSet<PathFragment> paths =
+        new ConcurrentSkipListSet<>(PathFragment.HIERARCHICAL_COMPARATOR);
 
     /**
      * Adds the given {@link ActionInput} to the trie.
@@ -389,8 +390,9 @@ void add(ActionInput input) {
 
     /** Checks whether the given {@link PathFragment} is contained in an artifact in the trie. */
     boolean contains(PathFragment execPath) {
-      // By the invariant of this set, if a prefix of execPath is present, it must sort right before
-      // it (or be equal to it).
+      // By the invariant of this set, there is at most one prefix of execPath in the set. Since the
+      // comparator sorts all children of a path right after the path itself, if such a prefix
+      // exists, it must thus sort right before execPath (or be equal to it).
       var floorPath = paths.floor(execPath);
       return floorPath != null && execPath.startsWith(floorPath);
     }

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

@@ -46,6 +46,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
+        "//src/main/java/com/google/devtools/build/lib/unsafe:string",
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
         "//src/main/java/com/google/devtools/build/lib/util:os",
         "//third_party:error_prone_annotations",

src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java

@@ -22,11 +22,14 @@
 import com.google.devtools.build.lib.skyframe.serialization.LeafSerializationContext;
 import com.google.devtools.build.lib.skyframe.serialization.SerializationException;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
+import com.google.devtools.build.lib.unsafe.StringUnsafe;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.errorprone.annotations.Immutable;
 import com.google.protobuf.CodedInputStream;
 import com.google.protobuf.CodedOutputStream;
 import java.io.IOException;
+import java.util.Arrays;
+import java.util.Comparator;
 import java.util.function.UnaryOperator;
 import javax.annotation.Nullable;
 
@@ -59,6 +62,56 @@ public abstract sealed class PathFragment
   public static final PathFragment EMPTY_FRAGMENT = new RelativePathFragment("");
 
   public static final char SEPARATOR_CHAR = '/';
+
+  /**
+   * Compares two path fragments lexicographically as sequences of case-sensitive path segments. The
+   * relative ordering of relative and absolute paths is unspecified.
+   *
+   * <p>The ordering imposed by this comparator differs from that of {@link
+   * #compareTo(PathFragment)} as it sorts {@code foo/bar-baz/quz} after {@code foo/bar/quz} - it
+   * has the property that the children of a path are sorted directly after their parent.
+   *
+   * <p>Note that the ordering imposed by this comparator is <em>not</em> consistent with equals if
+   * applied to paths that differ only in case on Windows. Paths of artifacts in a single build are
+   * known to not be affected by this as Bazel ensures that there is only a single artifact per
+   * equivalence class of {@link PathFragment}.
+   */
+  // TODO(bazel-team): Consider making this the default comparator for PathFragment and revisit the
+  //  choice to assume case sensitivity based on the host OS. Windows case sensitivity is
+  //  configurable on a per-directory basis:
+  //  https://learn.microsoft.com/en-us/windows/wsl/case-sensitivity
+  public static final Comparator<PathFragment> HIERARCHICAL_COMPARATOR =
+      (p1, p2) -> {
+        // Bazel's Strings contain raw UTF-8 bytes (see StringEncoding), which can be compared
+        // byte-by-byte.
+        var b1 = StringUnsafe.getInternalStringBytes(p1.getPathString());
+        var b2 = StringUnsafe.getInternalStringBytes(p2.getPathString());
+        // This is based on String.compareTo for the case of two Latin-1 coders.
+        int k = Arrays.mismatch(b1, b2);
+        if (k == -1) {
+          return 0;
+        }
+        if (k >= b1.length) {
+          // b1 is a prefix of b2.
+          return -1;
+        }
+        if (k >= b2.length) {
+          // b2 is a prefix of b1.
+          return 1;
+        }
+        byte c1 = b1[k];
+        byte c2 = b2[k];
+        if (c1 == '/') {
+          // Sort a/b/c before a/b-c.
+          return -1;
+        }
+        if (c2 == '/') {
+          // Sort a/b-c after a/b/c.
+          return 1;
+        }
+        return Byte.compareUnsigned(c1, c2);
+      };
+
   private static final char ADDITIONAL_SEPARATOR_CHAR = OS.additionalSeparator();
 
   private final String normalizedPath;
@@ -426,6 +479,14 @@ public int hashCode() {
     return OS.hash(this.normalizedPath);
   }
 
+  /**
+   * Compares this path fragment to another path fragment as normalized strings, possibly ignoring
+   * casing based on the host OS.
+   *
+   * <p>{@code dir/foo, dir/foo-bar/data.txt, dir/foo/data.txt} is sorted according to this method,
+   * which is not consistent with viewing a path as a sequence of segments. See {@link
+   * #HIERARCHICAL_COMPARATOR} for an alternative comparator.
+   */
   @Override
   public int compareTo(PathFragment o) {
     return OS.compare(this.normalizedPath, o.normalizedPath);
@@ -697,8 +758,8 @@ public String getDriveStr() {
   }
 
   /**
-   * Returns a relative PathFragment created from this absolute PathFragment using the
-   * same segments and drive letter.
+   * Returns a relative PathFragment created from this absolute PathFragment using the same segments
+   * and drive letter.
    */
   public PathFragment toRelative() {
     Preconditions.checkArgument(isAbsolute());

src/test/java/com/google/devtools/build/lib/remote/RemoteOutputCheckerTest.java

@@ -0,0 +1,58 @@
+// Copyright 2025 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.remote;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.ArtifactRoot;
+import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
+import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
+import com.google.devtools.build.lib.vfs.DigestHashFunction;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link RemoteOutputChecker} */
+@RunWith(JUnit4.class)
+public class RemoteOutputCheckerTest {
+  private final RemoteOutputChecker remoteOutputChecker =
+      new RemoteOutputChecker("build", RemoteOutputsMode.MINIMAL, ImmutableList.of());
+  private final FileSystem fs = new InMemoryFileSystem(DigestHashFunction.SHA256);
+  private final ArtifactRoot execRoot =
+      ArtifactRoot.asDerivedRoot(fs.getPath("/execroot"), ArtifactRoot.RootType.OUTPUT, "out");
+
+  @Test
+  public void testShouldDownloadOutput() {
+    remoteOutputChecker.addOutputToDownload(
+        ActionsTestUtil.createTreeArtifactWithGeneratingAction(execRoot, "foo/bar"));
+    remoteOutputChecker.addOutputToDownload(
+        ActionsTestUtil.createArtifact(execRoot, "foo/bar-baz"));
+    assertThat(
+            remoteOutputChecker.shouldDownloadOutput(PathFragment.create("out/foo/bar-quz"), null))
+        .isFalse();
+    assertThat(remoteOutputChecker.shouldDownloadOutput(PathFragment.create("out/foo/bar"), null))
+        .isTrue();
+    assertThat(
+            remoteOutputChecker.shouldDownloadOutput(
+                PathFragment.create("out/foo/bar/data.txt"), PathFragment.create("out/foo/bar")))
+        .isTrue();
+    assertThat(
+            remoteOutputChecker.shouldDownloadOutput(PathFragment.create("out/foo/bar-baz"), null))
+        .isTrue();
+  }
+}

src/test/java/com/google/devtools/build/lib/vfs/PathFragmentTest.java

@@ -16,6 +16,7 @@
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.vfs.PathFragment.EMPTY_FRAGMENT;
+import static com.google.devtools.build.lib.vfs.PathFragment.HIERARCHICAL_COMPARATOR;
 import static com.google.devtools.build.lib.vfs.PathFragment.create;
 import static org.junit.Assert.assertThrows;
 
@@ -497,7 +498,7 @@ private static ImmutableSet<PathFragment> toPathsSet(String... strs) {
 
   @Test
   public void testCompareTo() {
-    List<String> pathStrs =
+    ImmutableList<String> pathStrs =
         ImmutableList.of(
             "",
             "/",
@@ -550,6 +551,64 @@ public void testCompareTo() {
     assertThat(paths).isEqualTo(expectedOrder);
   }
 
+  @Test
+  public void testHierarchicalComparator() {
+    List<String> pathStrs =
+        ImmutableList.of(
+            "",
+            "/",
+            "foo",
+            "/foo",
+            "foo/bar",
+            "foo.bar",
+            "foo/bar.baz",
+            "foo/bar/baz",
+            "foo/barfile",
+            "foo/Bar",
+            "Foo/bar");
+    List<PathFragment> paths = toPaths(pathStrs);
+    // First test that compareTo is self-consistent.
+    for (PathFragment x : paths) {
+      for (PathFragment y : paths) {
+        for (PathFragment z : paths) {
+          // Anti-symmetry
+          assertThat(-1 * Integer.signum(HIERARCHICAL_COMPARATOR.compare(y, x)))
+              .isEqualTo(Integer.signum(HIERARCHICAL_COMPARATOR.compare(x, y)));
+          // Transitivity
+          if (HIERARCHICAL_COMPARATOR.compare(x, y) > 0
+              && HIERARCHICAL_COMPARATOR.compare(y, z) > 0) {
+            assertThat(HIERARCHICAL_COMPARATOR.compare(x, z)).isGreaterThan(0);
+          }
+          // "Substitutability"
+          if (HIERARCHICAL_COMPARATOR.compare(x, y) == 0) {
+            assertThat(Integer.signum(HIERARCHICAL_COMPARATOR.compare(y, z)))
+                .isEqualTo(Integer.signum(HIERARCHICAL_COMPARATOR.compare(x, z)));
+          }
+          // Consistency with equals
+          assertThat(x.equals(y)).isEqualTo(HIERARCHICAL_COMPARATOR.compare(x, y) == 0);
+        }
+      }
+    }
+    // Now test that compareTo does what we expect.  The exact ordering here doesn't matter much.
+    Collections.shuffle(paths);
+    paths.sort(HIERARCHICAL_COMPARATOR);
+    List<PathFragment> expectedOrder =
+        toPaths(
+            ImmutableList.of(
+                "",
+                "/",
+                "/foo",
+                "Foo/bar",
+                "foo",
+                "foo/Bar",
+                "foo/bar",
+                "foo/bar/baz",
+                "foo/bar.baz",
+                "foo/barfile",
+                "foo.bar"));
+    assertThat(paths).isEqualTo(expectedOrder);
+  }
+
   @Test
   public void testGetSafePathString() {
     assertThat(create("/").getSafePathString()).isEqualTo("/");
@@ -624,7 +683,7 @@ public void testSerializationSimple() throws Exception {
   @Test
   public void testSerializationAbsolute() throws Exception {
     checkSerialization("/foo");
-   }
+  }
 
   @Test
   public void testSerializationNested() throws Exception {