Clean up store hierarchy with `IndirectRootStore`

See the API doc comments for details.

12 files changed, 136 insertions, 37 deletions

diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc
index 214f3d49e..b7a27490c 100644
--- a/src/libstore/build/local-derivation-goal.cc
+++ b/src/libstore/build/local-derivation-goal.cc
@@ -1,5 +1,5 @@
 #include "local-derivation-goal.hh"
-#include "gc-store.hh"
+#include "indirect-root-store.hh"
 #include "hook-instance.hh"
 #include "worker.hh"
 #include "builtins.hh"
@@ -1200,7 +1200,7 @@ struct RestrictedStoreConfig : virtual LocalFSStoreConfig
 /* A wrapper around LocalStore that only allows building/querying of
    paths that are in the input closures of the build or were added via
    recursive Nix calls. */
-struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual LocalFSStore, public virtual GcStore
+struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual IndirectRootStore, public virtual GcStore
 {
     ref<LocalStore> next;
 
diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc
index ad3dee1a2..8cbf6f044 100644
--- a/src/libstore/daemon.cc
+++ b/src/libstore/daemon.cc
@@ -7,6 +7,7 @@
 #include "store-cast.hh"
 #include "gc-store.hh"
 #include "log-store.hh"
+#include "indirect-root-store.hh"
 #include "path-with-outputs.hh"
 #include "finally.hh"
 #include "archive.hh"
@@ -675,8 +676,8 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
         Path path = absPath(readString(from));
 
         logger->startWork();
-        auto & gcStore = require<GcStore>(*store);
-        gcStore.addIndirectRoot(path);
+        auto & indirectRootStore = require<IndirectRootStore>(*store);
+        indirectRootStore.addIndirectRoot(path);
         logger->stopWork();
 
         to << 1;
diff --git a/src/libstore/gc-store.hh b/src/libstore/gc-store.hh
index 2c26c65c4..ab1059fb1 100644
--- a/src/libstore/gc-store.hh
+++ b/src/libstore/gc-store.hh
@@ -71,20 +71,37 @@ struct GCResults
 };
 
 
+/**
+ * Mix-in class for \ref Store "stores" which expose a notion of garbage
+ * collection.
+ *
+ * Garbage collection will allow deleting paths which are not
+ * transitively "rooted".
+ *
+ * The notion of GC roots actually not part of this class.
+ *
+ *  - The base `Store` class has `Store::addTempRoot()` because for a store
+ *    that doesn't support garbage collection at all, a temporary GC root is
+ *    safely implementable as no-op.
+ *
+ *    @todo actually this is not so good because stores are *views*.
+ *    Some views have only a no-op temp roots even though others to the
+ *    same store allow triggering GC. For instance one can't add a root
+ *    over ssh, but that doesn't prevent someone from gc-ing that store
+ *    accesed via SSH locally).
+ *
+ *  - The derived `LocalFSStore` class has `LocalFSStore::addPermRoot`,
+ *    which is not part of this class because it relies on the notion of
+ *    an ambient file system. There are stores (`ssh-ng://`, for one),
+ *    that *do* support garbage collection but *don't* expose any file
+ *    system, and `LocalFSStore::addPermRoot` thus does not make sense
+ *    for them.
+ */
 struct GcStore : public virtual Store
 {
     inline static std::string operationName = "Garbage collection";
 
     /**
-     * Add an indirect root, which is merely a symlink to `path` from
-     * `/nix/var/nix/gcroots/auto/<hash of path>`.  `path` is supposed
-     * to be a symlink to a store path.  The garbage collector will
-     * automatically remove the indirect root when it finds that
-     * `path` has disappeared.
-     */
-    virtual void addIndirectRoot(const Path & path) = 0;
-
-    /**
      * Find the roots of the garbage collector.  Each root is a pair
      * `(link, storepath)` where `link` is the path of the symlink
      * outside of the Nix store that point to `storePath`. If
diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc
index 20720fb99..26c87391c 100644
--- a/src/libstore/gc.cc
+++ b/src/libstore/gc.cc
@@ -1,7 +1,6 @@
 #include "derivations.hh"
 #include "globals.hh"
 #include "local-store.hh"
-#include "local-fs-store.hh"
 #include "finally.hh"
 
 #include <functional>
@@ -50,7 +49,7 @@ void LocalStore::addIndirectRoot(const Path & path)
 }
 
 
-Path LocalFSStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot)
+Path IndirectRootStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot)
 {
     Path gcRoot(canonPath(_gcRoot));
 
diff --git a/src/libstore/indirect-root-store.hh b/src/libstore/indirect-root-store.hh
new file mode 100644
index 000000000..59e45af45
--- /dev/null
+++ b/src/libstore/indirect-root-store.hh
@@ -0,0 +1,48 @@
+#pragma once
+///@file
+
+#include "local-fs-store.hh"
+
+namespace nix {
+
+/**
+ * Mix-in class for implementing permanent roots as a pair of a direct
+ * (strong) reference and indirect weak reference to the first
+ * reference.
+ *
+ * See methods for details on the operations it represents.
+ */
+struct IndirectRootStore : public virtual LocalFSStore
+{
+    inline static std::string operationName = "Indirect GC roots registration";
+
+    /**
+     * Implementation of `LocalFSStore::addPermRoot` where the permanent
+     * root is a pair of
+     *
+     * - The user-facing symlink which all implementations must create
+     *
+     * - An additional weak reference known as the "indirect root" that
+     *   points to that symlink.
+     *
+     * The garbage collector will automatically remove the indirect root
+     * when it finds that the symlink has disappeared.
+     *
+     * The implementation of this method is concrete, but it delegates
+     * to `addIndirectRoot()` which is abstract.
+     */
+    Path addPermRoot(const StorePath & storePath, const Path & gcRoot) override final;
+
+    /**
+     * Add an indirect root, which is a weak reference to the
+     * user-facing symlink created by `addPermRoot()`.
+     *
+     * @param path user-facing and user-controlled symlink to a store
+     * path.
+     *
+     * The form this weak-reference takes is implementation-specific.
+     */
+    virtual void addIndirectRoot(const Path & path) = 0;
+};
+
+}
diff --git a/src/libstore/local-fs-store.hh b/src/libstore/local-fs-store.hh
index 2ee2ef0c8..488109501 100644
--- a/src/libstore/local-fs-store.hh
+++ b/src/libstore/local-fs-store.hh
@@ -40,6 +40,7 @@ class LocalFSStore : public virtual LocalFSStoreConfig,
     public virtual LogStore
 {
 public:
+    inline static std::string operationName = "Local Filesystem Store";
 
     const static std::string drvsLogDir;
 
@@ -49,9 +50,20 @@ public:
     ref<FSAccessor> getFSAccessor() override;
 
     /**
-     * Register a permanent GC root.
+     * Creates symlink from the `gcRoot` to the `storePath` and
+     * registers the `gcRoot` as a permanent GC root. The `gcRoot`
+     * symlink lives outside the store and is created and owned by the
+     * user.
+     *
+     * @param gcRoot The location of the symlink.
+     *
+     * @param storePath The store object being rooted. The symlink will
+     * point to `toRealPath(store.printStorePath(storePath))`.
+     *
+     * How the permanent GC root corresponding to this symlink is
+     * managed is implementation-specific.
      */
-    Path addPermRoot(const StorePath & storePath, const Path & gcRoot);
+    virtual Path addPermRoot(const StorePath & storePath, const Path & gcRoot) = 0;
 
     virtual Path getRealStoreDir() { return realStoreDir; }
 
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index 2bdd46fa2..4125cacf4 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -5,8 +5,7 @@
 
 #include "pathlocks.hh"
 #include "store-api.hh"
-#include "local-fs-store.hh"
-#include "gc-store.hh"
+#include "indirect-root-store.hh"
 #include "sync.hh"
 #include "util.hh"
 
@@ -68,7 +67,9 @@ struct LocalStoreConfig : virtual LocalFSStoreConfig
     std::string doc() override;
 };
 
-class LocalStore : public virtual LocalStoreConfig, public virtual LocalFSStore, public virtual GcStore
+class LocalStore : public virtual LocalStoreConfig
+    , public virtual IndirectRootStore
+    , public virtual GcStore
 {
 private:
 
@@ -209,6 +210,12 @@ private:
 
 public:
 
+    /**
+     * Implementation of IndirectRootStore::addIndirectRoot().
+     *
+     * The weak reference merely is a symlink to `path' from
+     * /nix/var/nix/gcroots/auto/<hash of `path'>.
+     */
     void addIndirectRoot(const Path & path) override;
 
 private:
diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
index 926ccd9d1..21258daec 100644
--- a/src/libstore/remote-store.cc
+++ b/src/libstore/remote-store.cc
@@ -822,15 +822,6 @@ void RemoteStore::addTempRoot(const StorePath & path)
 }
 
 
-void RemoteStore::addIndirectRoot(const Path & path)
-{
-    auto conn(getConnection());
-    conn->to << WorkerProto::Op::AddIndirectRoot << path;
-    conn.processStderr();
-    readInt(conn->from);
-}
-
-
 Roots RemoteStore::findRoots(bool censor)
 {
     auto conn(getConnection());
diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh
index 68ec29e6c..a1ae82a0f 100644
--- a/src/libstore/remote-store.hh
+++ b/src/libstore/remote-store.hh
@@ -126,8 +126,6 @@ public:
 
     void addTempRoot(const StorePath & path) override;
 
-    void addIndirectRoot(const Path & path) override;
-
     Roots findRoots(bool censor) override;
 
     void collectGarbage(const GCOptions & options, GCResults & results) override;
diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc
index 0200076c0..9c6c42ef4 100644
--- a/src/libstore/ssh-store.cc
+++ b/src/libstore/ssh-store.cc
@@ -1,5 +1,6 @@
 #include "ssh-store-config.hh"
 #include "store-api.hh"
+#include "local-fs-store.hh"
 #include "remote-store.hh"
 #include "remote-store-connection.hh"
 #include "remote-fs-accessor.hh"
@@ -61,7 +62,7 @@ public:
     std::optional<std::string> getBuildLogExact(const StorePath & path) override
     { unsupported("getBuildLogExact"); }
 
-private:
+protected:
 
     struct Connection : RemoteStore::Connection
     {
@@ -93,9 +94,12 @@ private:
 ref<RemoteStore::Connection> SSHStore::openConnection()
 {
     auto conn = make_ref<Connection>();
-    conn->sshConn = master.startCommand(
-        fmt("%s --stdio", remoteProgram)
-        + (remoteStore.get() == "" ? "" : " --store " + shellEscape(remoteStore.get())));
+
+    std::string command = remoteProgram + " --stdio";
+    if (remoteStore.get() != "")
+        command += " --store " + shellEscape(remoteStore.get());
+
+    conn->sshConn = master.startCommand(command);
     conn->to = FdSink(conn->sshConn->in.get());
     conn->from = FdSource(conn->sshConn->out.get());
     return conn;
diff --git a/src/libstore/uds-remote-store.cc b/src/libstore/uds-remote-store.cc
index 69dae2da5..99589f8b2 100644
--- a/src/libstore/uds-remote-store.cc
+++ b/src/libstore/uds-remote-store.cc
@@ -1,4 +1,5 @@
 #include "uds-remote-store.hh"
+#include "worker-protocol.hh"
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -77,6 +78,15 @@ ref<RemoteStore::Connection> UDSRemoteStore::openConnection()
 }
 
 
+void UDSRemoteStore::addIndirectRoot(const Path & path)
+{
+    auto conn(getConnection());
+    conn->to << WorkerProto::Op::AddIndirectRoot << path;
+    conn.processStderr();
+    readInt(conn->from);
+}
+
+
 static RegisterStoreImplementation<UDSRemoteStore, UDSRemoteStoreConfig> regUDSRemoteStore;
 
 }
diff --git a/src/libstore/uds-remote-store.hh b/src/libstore/uds-remote-store.hh
index b9d9a39b5..cdb28a001 100644
--- a/src/libstore/uds-remote-store.hh
+++ b/src/libstore/uds-remote-store.hh
@@ -3,7 +3,7 @@
 
 #include "remote-store.hh"
 #include "remote-store-connection.hh"
-#include "local-fs-store.hh"
+#include "indirect-root-store.hh"
 
 namespace nix {
 
@@ -21,7 +21,9 @@ struct UDSRemoteStoreConfig : virtual LocalFSStoreConfig, virtual RemoteStoreCon
     std::string doc() override;
 };
 
-class UDSRemoteStore : public virtual UDSRemoteStoreConfig, public virtual LocalFSStore, public virtual RemoteStore
+class UDSRemoteStore : public virtual UDSRemoteStoreConfig
+    , public virtual IndirectRootStore
+    , public virtual RemoteStore
 {
 public:
 
@@ -39,6 +41,16 @@ public:
     void narFromPath(const StorePath & path, Sink & sink) override
     { LocalFSStore::narFromPath(path, sink); }
 
+    /**
+     * Implementation of `IndirectRootStore::addIndirectRoot()` which
+     * delegates to the remote store.
+     *
+     * The idea is that the client makes the direct symlink, so it is
+     * owned managed by the client's user account, and the server makes
+     * the indirect symlink.
+     */
+    void addIndirectRoot(const Path & path) override;
+
 private:
 
     struct Connection : RemoteStore::Connection