Use ctime in file digest cache key

File digests are now additionally keyed by ctime for supported file system implementations. Since Bazel has a non-zero default for `--cache_computed_file_digests`, this may be required for correctness in cases where different files have identical mtime and inode number. For example, this can happen on Linux when files are extracted from a tar file with fixed mtime and are then replaced with `mv`, which preserves inodes.

Since Java (N)IO doesn't have support for reading file ctimes on Windows, a new method backed by a native implementation is added to `WindowsFileOperation`. Adding a call to this function to `stat` uncovered previously silent bugs where Unix-style `PathFragment`s were created on Windows:

1. Bzlmod's `createLocalRepoSpec` did not correctly extract the path from a registry's `file://` URI on Windows.
2. `--package_path` isn't usable with absolute paths on Windows as it splits on `:`. Since the flag is deprecated, this commit fixes the tests rather than the implementation.

Fixes #14723

Closes #18003.

PiperOrigin-RevId: 524297459
Change-Id: I96bfc0210e2f71bf8603c7b7cc5eb06a04048c85

Author: fmeum

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD

@@ -77,6 +77,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/events",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//third_party:gson",
         "//third_party:guava",

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java

@@ -24,6 +24,7 @@
 import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.gson.FieldNamingPolicy;
 import com.google.gson.Gson;
@@ -176,7 +177,15 @@ private RepoSpec createLocalPathRepoSpec(
       path = moduleBase + "/" + path;
       if (!PathFragment.isAbsolute(moduleBase)) {
         if (uri.getScheme().equals("file")) {
-          path = uri.getPath() + "/" + path;
+          if (uri.getPath().isEmpty() || !uri.getPath().startsWith("/")) {
+            throw new IOException(
+                String.format(
+                    "Provided non absolute local registry path for module %s: %s",
+                    key, uri.getPath()));
+          }
+          // Unix:    file:///tmp --> /tmp
+          // Windows: file:///C:/tmp --> C:/tmp
+          path = uri.getPath().substring(OS.getCurrent() == OS.WINDOWS ? 1 : 0) + "/" + path;
         } else {
           throw new IOException(String.format("Provided non local registry for module %s", key));
         }

src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java

@@ -692,7 +692,10 @@ private static FileArtifactValue fileArtifactValueFromStat(
   }
 
   private void setPathPermissionsIfFile(Path path) throws IOException {
-    if (path.isFile(Symlinks.NOFOLLOW)) {
+    FileStatus stat = path.statIfFound(Symlinks.NOFOLLOW);
+    if (stat != null
+        && stat.isFile()
+        && stat.getPermissions() != outputPermissions.getPermissionsMode()) {
       setPathPermissions(path);
     }
   }

src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java

@@ -120,7 +120,8 @@ public long getNodeId() {
       return status.getInodeNumber();
     }
 
-    int getPermissions() {
+    @Override
+    public int getPermissions() {
       return status.getPermissions();
     }
 

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

@@ -46,6 +46,9 @@ private static class CacheKey {
     /** File system identifier of the file (typically the inode number). */
     private final long nodeId;
 
+    /** Last change time of the file. */
+    private final long changeTime;
+
     /** Last modification time of the file. */
     private final long modifiedTime;
 
@@ -62,6 +65,7 @@ private static class CacheKey {
     public CacheKey(Path path, FileStatus status) throws IOException {
       this.path = path.asFragment();
       this.nodeId = status.getNodeId();
+      this.changeTime = status.getLastChangeTime();
       this.modifiedTime = status.getLastModifiedTime();
       this.size = status.getSize();
     }
@@ -76,6 +80,7 @@ public boolean equals(Object object) {
         CacheKey key = (CacheKey) object;
         return path.equals(key.path)
             && nodeId == key.nodeId
+            && changeTime == key.changeTime
             && modifiedTime == key.modifiedTime
             && size == key.size;
       }
@@ -86,6 +91,7 @@ public int hashCode() {
       int result = 17;
       result = 31 * result + path.hashCode();
       result = 31 * result + Longs.hashCode(nodeId);
+      result = 31 * result + Longs.hashCode(changeTime);
       result = 31 * result + Longs.hashCode(modifiedTime);
       result = 31 * result + Longs.hashCode(size);
       return result;

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

@@ -85,4 +85,16 @@ public interface FileStatus {
    * ought to cause the node ID of b to change, but appending / modifying b should not.
    */
   long getNodeId() throws IOException;
+
+  /**
+   * Returns the file's permissions in POSIX format (e.g. 0755) if possible without performing
+   * additional IO, otherwise (or if unsupported by the file system) returns -1.
+   *
+   * <p>If accurate group and other permissions aren't available, the returned value should attempt
+   * to mimic a umask of 022 (i.e. read and execute permissions extend to group and other, write
+   * does not).
+   */
+  default int getPermissions() {
+    return -1;
+  }
 }

src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryContentInfo.java

@@ -120,6 +120,22 @@ public long getNodeId() {
     return System.identityHashCode(this);
   }
 
+  @Override
+  public final int getPermissions() {
+    int permissions = 0;
+    // Emulate the default umask of 022.
+    if (isReadable) {
+      permissions |= 0444;
+    }
+    if (isWritable) {
+      permissions |= 0200;
+    }
+    if (isExecutable) {
+      permissions |= 0111;
+    }
+    return permissions;
+  }
+
   @Override
   public final InMemoryContentInfo inode() {
     return this;

src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java

@@ -15,7 +15,9 @@
 package com.google.devtools.build.lib.windows;
 
 import com.google.devtools.build.lib.jni.JniLoader;
+import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.nio.file.AccessDeniedException;
 
 /** File operations on Windows. */
 public class WindowsFileOperations {
@@ -82,6 +84,12 @@ public Status getStatus() {
   // IS_SYMLINK_OR_JUNCTION_ERROR = 1;
   private static final int IS_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST = 2;
 
+  // Keep GET_CHANGE_TIME_* values in sync with src/main/native/windows/file.cc.
+  private static final int GET_CHANGE_TIME_SUCCESS = 0;
+  //  private static final int GET_CHANGE_TIME_ERROR = 1;
+  private static final int GET_CHANGE_TIME_DOES_NOT_EXIST = 2;
+  private static final int GET_CHANGE_TIME_ACCESS_DENIED = 3;
+
   // Keep CREATE_JUNCTION_* values in sync with src/main/native/windows/file.h.
   private static final int CREATE_JUNCTION_SUCCESS = 0;
   // CREATE_JUNCTION_ERROR = 1;
@@ -114,6 +122,9 @@ public Status getStatus() {
   private static native int nativeIsSymlinkOrJunction(
       String path, boolean[] result, String[] error);
 
+  private static native int nativeGetChangeTime(
+      String path, boolean followReparsePoints, long[] result, String[] error);
+
   private static native boolean nativeGetLongPath(String path, String[] result, String[] error);
 
   private static native int nativeCreateJunction(String name, String target, String[] error);
@@ -143,6 +154,25 @@ public static boolean isSymlinkOrJunction(String path) throws IOException {
     throw new IOException(String.format("Cannot tell if '%s' is link: %s", path, error[0]));
   }
 
+  /** Returns the time at which the file was last changed, including metadata changes. */
+  public static long getLastChangeTime(String path, boolean followReparsePoints)
+      throws IOException {
+    long[] result = new long[] {0};
+    String[] error = new String[] {null};
+    switch (nativeGetChangeTime(asLongPath(path), followReparsePoints, result, error)) {
+      case GET_CHANGE_TIME_SUCCESS:
+        return result[0];
+      case GET_CHANGE_TIME_DOES_NOT_EXIST:
+        throw new FileNotFoundException(path);
+      case GET_CHANGE_TIME_ACCESS_DENIED:
+        throw new AccessDeniedException(path);
+      default:
+        // This is GET_CHANGE_TIME_ERROR (1). The JNI code puts a custom message in 'error[0]'.
+        break;
+    }
+    throw new IOException(String.format("Cannot get last change time of '%s': %s", path, error[0]));
+  }
+
   /**
    * Returns the long path associated with the input `path`.
    *

src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java

@@ -156,6 +156,8 @@ protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOEx
     }
 
     final boolean isSymbolicLink = !followSymlinks && fileIsSymbolicLink(file);
+    final long lastChangeTime =
+        WindowsFileOperations.getLastChangeTime(getNioPath(path).toString(), followSymlinks);
     FileStatus status =
         new FileStatus() {
           @Override
@@ -193,15 +195,20 @@ public long getLastModifiedTime() {
 
           @Override
           public long getLastChangeTime() {
-            // This is the best we can do with Java NIO...
-            return attributes.lastModifiedTime().toMillis();
+            return lastChangeTime;
           }
 
           @Override
           public long getNodeId() {
             // TODO(bazel-team): Consider making use of attributes.fileKey().
             return -1;
           }
+
+          @Override
+          public int getPermissions() {
+            // Files on Windows are implicitly readable and executable.
+            return 0555 | (attributes.isReadOnly() ? 0 : 0200);
+          }
         };
 
     return status;

src/main/native/windows/file-jni.cc

@@ -62,6 +62,29 @@ Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeIsSymlink
   return static_cast<jint>(result);
 }
 
+extern "C" JNIEXPORT jint JNICALL
+Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeGetChangeTime(
+    JNIEnv* env, jclass clazz, jstring path, jboolean follow_reparse_points,
+    jlongArray result_holder, jobjectArray error_msg_holder) {
+  std::wstring wpath(bazel::windows::GetJavaWstring(env, path));
+  std::wstring error;
+  jlong ctime = 0;
+  int result =
+      bazel::windows::GetChangeTime(wpath.c_str(), follow_reparse_points,
+                                    reinterpret_cast<int64_t*>(&ctime), &error);
+  if (result == bazel::windows::GetChangeTimeResult::kSuccess) {
+    env->SetLongArrayRegion(result_holder, 0, 1, &ctime);
+  } else {
+    if (!error.empty() && CanReportError(env, error_msg_holder)) {
+      ReportLastError(
+          bazel::windows::MakeErrorMessage(
+              WSTR(__FILE__), __LINE__, L"nativeGetChangeTime", wpath, error),
+          env, error_msg_holder);
+    }
+  }
+  return static_cast<jint>(result);
+}
+
 extern "C" JNIEXPORT jboolean JNICALL
 Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeGetLongPath(
     JNIEnv* env, jclass clazz, jstring path, jobjectArray result_holder,