Fix very slow remote temp root registration in copyPaths()

src/libstore/build/derivation-goal.cc

@@ -78,9 +78,12 @@ Goal::Co DerivationGoal::haveDerivation(bool storeDerivation)
     if (!drv->type().hasKnownOutputPaths())
         experimentalFeatureSettings.require(Xp::CaDerivations);
 
+    StorePathSet outputPaths;
     for (auto & i : drv->outputsAndOptPaths(worker.store))
         if (i.second.second)
-            worker.store.addTempRoot(*i.second.second);
+            outputPaths.insert(*i.second.second);
+
+    worker.store.addTempRoots(outputPaths);
 
     /* We don't yet have any safe way to cache an impure derivation at
        this step. */

src/libstore/daemon.cc

@@ -695,6 +695,15 @@ static void performOp(
         break;
     }
 
+    case WorkerProto::Op::AddTempRoots: {
+        auto paths = WorkerProto::Serialise<StorePathSet>::read(*store, rconn);
+        logger->startWork();
+        store->addTempRoots(paths);
+        logger->stopWork();
+        conn.to << 1;
+        break;
+    }
+
     case WorkerProto::Op::AddPermRoot: {
         if (!trusted)
             throw Error(

src/libstore/gc.cc

@@ -75,7 +75,7 @@ void LocalStore::createTempRootsFile()
     }
 }
 
-void LocalStore::addTempRoot(const StorePath & path)
+void LocalStore::addTempRoots(const StorePathSet & paths)
 {
     if (config->readOnly) {
         debug(
@@ -124,12 +124,14 @@ void LocalStore::addTempRoot(const StorePath & path)
         }
 
         try {
-            debug("sending GC root '%s'", printStorePath(path));
-            writeFull(fdRootsSocket->get(), printStorePath(path) + "\n", false);
-            char c;
-            readFull(fdRootsSocket->get(), &c, 1);
-            assert(c == '1');
-            debug("got ack for GC root '%s'", printStorePath(path));
+            for (auto & path : paths) {
+                debug("sending GC root '%s'", printStorePath(path));
+                writeFull(fdRootsSocket->get(), printStorePath(path) + "\n", false);
+                char c;
+                readFull(fdRootsSocket->get(), &c, 1);
+                assert(c == '1');
+                debug("got ack for GC root '%s'", printStorePath(path));
+            }
         } catch (SystemError & e) {
             /* The garbage collector may have exited, so we need to
                restart. */
@@ -146,10 +148,22 @@ void LocalStore::addTempRoot(const StorePath & path)
         }
     }
 
-    /* Record the store path in the temporary roots file so it will be
+    /* Record the store paths in the temporary roots file so they will be
        seen by a future run of the garbage collector. */
-    auto s = printStorePath(path) + '\0';
-    writeFull(_fdTempRoots.lock()->get(), s);
+
+    std::string s;
+
+    for (auto & path : paths)
+        s += printStorePath(path) + '\0';
+
+    {
+        auto fdTempRoots(_fdTempRoots.lock());
+
+        /* This might not be atomic, but that's fine. Writes go in-order, and if
+           we partially write a store path, findTempRoots() will just ignore it,
+           and we'll send it the new temproots below if it's still running. */
+        writeFull(fdTempRoots->get(), s);
+    }
 }
 
 static std::string censored = "{censored}";

src/libstore/include/nix/store/gc-store.hh

@@ -93,7 +93,7 @@ struct GCResults
  *
  * The notion of GC roots actually not part of this class.
  *
- *  - The base `Store` class has `Store::addTempRoot()` because for a store
+ *  - The base `Store` class has `Store::addTempRoots()` because for a store
  *    that doesn't support garbage collection at all, a temporary GC root is
  *    safely implementable as no-op.
  *

src/libstore/include/nix/store/local-store.hh

@@ -299,7 +299,7 @@ public:
         RepairFlag repair,
         std::shared_ptr<const Provenance> provenance) override;
 
-    void addTempRoot(const StorePath & path) override;
+    void addTempRoots(const StorePathSet & paths) override;
 
 private:
 

src/libstore/include/nix/store/remote-store.hh

@@ -121,7 +121,7 @@ struct RemoteStore : public virtual Store,
 
     void ensurePath(const StorePath & path) override;
 
-    void addTempRoot(const StorePath & path) override;
+    void addTempRoots(const StorePathSet & paths) override;
 
     Roots findRoots(bool censor) override;
 

src/libstore/include/nix/store/store-api.hh

@@ -727,9 +727,18 @@ public:
      * Add a store path as a temporary root of the garbage collector.
      * The root disappears as soon as we exit.
      */
-    virtual void addTempRoot(const StorePath & path)
+    void addTempRoot(const StorePath & path)
     {
-        debug("not creating temporary root, store doesn't support GC");
+        addTempRoots({path});
+    }
+
+    /**
+     * Add multiple store paths as temporary roots of the garbage collector.
+     * The roots disappears as soon as we exit.
+     */
+    virtual void addTempRoots(const StorePathSet & paths)
+    {
+        debug("not creating temporary roots, store doesn't support GC");
     }
 
     /**

src/libstore/include/nix/store/worker-protocol.hh

@@ -113,6 +113,7 @@ struct WorkerProto
     static constexpr std::string_view featureQueryActiveBuilds = "queryActiveBuilds";
     static constexpr std::string_view featureProvenance = "provenance";
     static constexpr std::string_view featureVersionedAddToStoreMultiple = "versionedAddToStoreMultiple";
+    static constexpr std::string_view featureAddTempRoots = "addTempRoots";
 
     /**
      * A unidirectional read connection, to be used by the read half of the
@@ -238,6 +239,7 @@ enum struct WorkerProto::Op : uint64_t {
     BuildPathsWithResults = 46,
     AddPermRoot = 47,
     QueryActiveBuilds = 48,
+    AddTempRoots = 49,
 };
 
 struct WorkerProto::ClientHandshakeInfo

src/libstore/remote-store.cc

@@ -693,10 +693,26 @@ void RemoteStore::ensurePath(const StorePath & path)
     readInt(conn->from);
 }
 
-void RemoteStore::addTempRoot(const StorePath & path)
+void RemoteStore::addTempRoots(const StorePathSet & paths)
 {
+    if (paths.empty())
+        return;
+
     auto conn(getConnection());
-    conn->addTempRoot(*this, &conn.daemonException, path);
+
+    if (conn->protoVersion.features.contains(WorkerProto::featureAddTempRoots)) {
+        conn->to << WorkerProto::Op::AddTempRoots;
+        WorkerProto::write(*this, *conn, paths);
+        conn.processStderr();
+        readInt(conn->from);
+    } else {
+        /* Fallback for daemons that don't support the batched
+           operation. Note that this is very slow for large sets of
+           paths on high-latency links, due to a network round-trip per
+           path. */
+        for (auto & path : paths)
+            conn->addTempRoot(*this, &conn.daemonException, path);
+    }
 }
 
 Roots RemoteStore::findRoots(bool censor)

src/libstore/restricted-store.cc

@@ -124,7 +124,7 @@ struct RestrictedStore : public virtual IndirectRootStore, public virtual GcStor
         unsupported("buildDerivation");
     }
 
-    void addTempRoot(const StorePath & path) override {}
+    void addTempRoots(const StorePathSet & paths) override {}
 
     void addIndirectRoot(const std::filesystem::path & path) override {}
 

src/libstore/store-api.cc

@@ -974,8 +974,7 @@ std::map<StorePath, StorePath> copyPaths(
     CheckSigsFlag checkSigs,
     SubstituteFlag substitute)
 {
-    for (auto & path : storePaths)
-        dstStore.addTempRoot(path);
+    dstStore.addTempRoots(storePaths);
 
     auto valid = dstStore.queryValidPaths(storePaths, substitute);
 

src/libstore/worker-protocol.cc

@@ -26,6 +26,7 @@ const WorkerProto::Version WorkerProto::latest = {
             std::string{WorkerProto::featureQueryActiveBuilds},
             std::string{WorkerProto::featureProvenance},
             std::string{WorkerProto::featureVersionedAddToStoreMultiple},
+            std::string{WorkerProto::featureAddTempRoots},
         },
 };
 

src/nix/nix-store/nix-store.cc

@@ -953,8 +953,7 @@ static void opServe(Strings opFlags, Strings opArgs)
             bool substitute = readInt(in);
             auto paths = ServeProto::Serialise<StorePathSet>::read(*store, rconn);
             if (lock && writeAllowed)
-                for (auto & path : paths)
-                    store->addTempRoot(path);
+                store->addTempRoots(paths);
 
             if (substitute && writeAllowed) {
                 store->substitutePaths(paths);