From e95503cf9abab2fc724bf0607256bd0f1efd6b26 Mon Sep 17 00:00:00 2001
From: Sergei Zimmerman <sergei@zimmerman.foo>
Date: Thu, 13 Nov 2025 23:54:14 +0300
Subject: [PATCH] libutil: Make PosixSourceAccessor update mtime only when
 needed

Typically PosixSourceAccessor can be used from multiple threads,
but mtime is not updated atomically (i.e. with compare_exchange_weak),
so mtime gets raced. It's only needed in dumpPathAndGetMtime and mtime
tracking can be gated behind that.

Also start using getLastModified interface instead of dynamic casts.
---
 src/libutil/archive.cc                         |  4 ++--
 .../include/nix/util/posix-source-accessor.hh  | 18 +++++++++++++++---
 src/libutil/posix-source-accessor.cc           | 16 ++++++++++------
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc
index 737d9b2fe..3b5b610db 100644
--- a/src/libutil/archive.cc
+++ b/src/libutil/archive.cc
@@ -103,9 +103,9 @@ void SourceAccessor::dumpPath(const CanonPath & path, Sink & sink, PathFilter &
 
 time_t dumpPathAndGetMtime(const Path & path, Sink & sink, PathFilter & filter)
 {
-    auto path2 = PosixSourceAccessor::createAtRoot(path);
+    auto path2 = PosixSourceAccessor::createAtRoot(path, /*trackLastModified=*/true);
     path2.dumpPath(sink, filter);
-    return path2.accessor.dynamic_pointer_cast<PosixSourceAccessor>()->mtime;
+    return path2.accessor->getLastModified().value();
 }
 
 void dumpPath(const Path & path, Sink & sink, PathFilter & filter)
diff --git a/src/libutil/include/nix/util/posix-source-accessor.hh b/src/libutil/include/nix/util/posix-source-accessor.hh
index 895e2e1c1..29561a3da 100644
--- a/src/libutil/include/nix/util/posix-source-accessor.hh
+++ b/src/libutil/include/nix/util/posix-source-accessor.hh
@@ -9,7 +9,7 @@ struct SourcePath;
 /**
  * A source accessor that uses the Unix filesystem.
  */
-struct PosixSourceAccessor : virtual SourceAccessor
+class PosixSourceAccessor : virtual public SourceAccessor
 {
     /**
      * Optional root path to prefix all operations into the native file
@@ -18,8 +18,12 @@ struct PosixSourceAccessor : virtual SourceAccessor
      */
     const std::filesystem::path root;
 
+    const bool trackLastModified = false;
+
+public:
+
     PosixSourceAccessor();
-    PosixSourceAccessor(std::filesystem::path && root);
+    PosixSourceAccessor(std::filesystem::path && root, bool trackLastModified = false);
 
     /**
      * The most recent mtime seen by lstat(). This is a hack to
@@ -43,6 +47,9 @@ struct PosixSourceAccessor : virtual SourceAccessor
      * Create a `PosixSourceAccessor` and `SourcePath` corresponding to
      * some native path.
      *
+     * @param Whether the accessor should return a non-null getLastModified.
+     * When true the accessor must be used only by a single thread.
+     *
      * The `PosixSourceAccessor` is rooted as far up the tree as
      * possible, (e.g. on Windows it could scoped to a drive like
      * `C:\`). This allows more `..` parent accessing to work.
@@ -64,7 +71,12 @@ struct PosixSourceAccessor : virtual SourceAccessor
      * and
      * [`std::filesystem::path::relative_path`](https://en.cppreference.com/w/cpp/filesystem/path/relative_path).
      */
-    static SourcePath createAtRoot(const std::filesystem::path & path);
+    static SourcePath createAtRoot(const std::filesystem::path & path, bool trackLastModified = false);
+
+    std::optional<std::time_t> getLastModified() override
+    {
+        return trackLastModified ? std::optional{mtime} : std::nullopt;
+    }
 
 private:
 
diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc
index fe3bcb1c1..abbab45db 100644
--- a/src/libutil/posix-source-accessor.cc
+++ b/src/libutil/posix-source-accessor.cc
@@ -7,8 +7,9 @@
 
 namespace nix {
 
-PosixSourceAccessor::PosixSourceAccessor(std::filesystem::path && argRoot)
+PosixSourceAccessor::PosixSourceAccessor(std::filesystem::path && argRoot, bool trackLastModified)
     : root(std::move(argRoot))
+    , trackLastModified(trackLastModified)
 {
     assert(root.empty() || root.is_absolute());
     displayPrefix = root.string();
@@ -19,11 +20,11 @@ PosixSourceAccessor::PosixSourceAccessor()
 {
 }
 
-SourcePath PosixSourceAccessor::createAtRoot(const std::filesystem::path & path)
+SourcePath PosixSourceAccessor::createAtRoot(const std::filesystem::path & path, bool trackLastModified)
 {
     std::filesystem::path path2 = absPath(path);
     return {
-        make_ref<PosixSourceAccessor>(path2.root_path()),
+        make_ref<PosixSourceAccessor>(path2.root_path(), trackLastModified),
         CanonPath{path2.relative_path().string()},
     };
 }
@@ -114,9 +115,12 @@ std::optional<SourceAccessor::Stat> PosixSourceAccessor::maybeLstat(const CanonP
     auto st = cachedLstat(path);
     if (!st)
         return std::nullopt;
-    // This makes the accessor thread-unsafe, but we only seem to use the actual value in a single threaded context in
-    // `src/libfetchers/path.cc`.
-    mtime = std::max(mtime, st->st_mtime);
+
+    /* The contract is that trackLastModified implies that the caller uses the accessor
+       from a single thread. Thus this is not a CAS loop. */
+    if (trackLastModified)
+        mtime = std::max(mtime, st->st_mtime);
+
     return Stat{
         .type = S_ISREG(st->st_mode)   ? tRegular
                 : S_ISDIR(st->st_mode) ? tDirectory