Merge pull request #8243 from obsidiansystems/indirect-root-store

Refactor `Store` hierarchy with a new `IndirectRootStore` interface

14 files changed, 186 insertions, 82 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-connection.hh b/src/libstore/remote-store-connection.hh
index d32d91a60..ce4740a9c 100644
--- a/src/libstore/remote-store-connection.hh
+++ b/src/libstore/remote-store-connection.hh
@@ -1,5 +1,6 @@
 #include "remote-store.hh"
 #include "worker-protocol.hh"
+#include "pool.hh"
 
 namespace nix {
 
@@ -94,4 +95,34 @@ struct RemoteStore::Connection
     std::exception_ptr processStderr(Sink * sink = 0, Source * source = 0, bool flush = true);
 };
 
+/**
+ * A wrapper around Pool<RemoteStore::Connection>::Handle that marks
+ * the connection as bad (causing it to be closed) if a non-daemon
+ * exception is thrown before the handle is closed. Such an exception
+ * causes a deviation from the expected protocol and therefore a
+ * desynchronization between the client and daemon.
+ */
+struct RemoteStore::ConnectionHandle
+{
+    Pool<RemoteStore::Connection>::Handle handle;
+    bool daemonException = false;
+
+    ConnectionHandle(Pool<RemoteStore::Connection>::Handle && handle)
+        : handle(std::move(handle))
+    { }
+
+    ConnectionHandle(ConnectionHandle && h)
+        : handle(std::move(h.handle))
+    { }
+
+    ~ConnectionHandle();
+
+    RemoteStore::Connection & operator * () { return *handle; }
+    RemoteStore::Connection * operator -> () { return &*handle; }
+
+    void processStderr(Sink * sink = 0, Source * source = 0, bool flush = true);
+
+    void withFramedSink(std::function<void(Sink & sink)> fun);
+};
+
 }
diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
index bfe2258a4..21258daec 100644
--- a/src/libstore/remote-store.cc
+++ b/src/libstore/remote-store.cc
@@ -159,49 +159,25 @@ void RemoteStore::setOptions(Connection & conn)
 }
 
 
-/* A wrapper around Pool<RemoteStore::Connection>::Handle that marks
-   the connection as bad (causing it to be closed) if a non-daemon
-   exception is thrown before the handle is closed. Such an exception
-   causes a deviation from the expected protocol and therefore a
-   desynchronization between the client and daemon. */
-struct ConnectionHandle
+RemoteStore::ConnectionHandle::~ConnectionHandle()
 {
-    Pool<RemoteStore::Connection>::Handle handle;
-    bool daemonException = false;
-
-    ConnectionHandle(Pool<RemoteStore::Connection>::Handle && handle)
-        : handle(std::move(handle))
-    { }
-
-    ConnectionHandle(ConnectionHandle && h)
-        : handle(std::move(h.handle))
-    { }
-
-    ~ConnectionHandle()
-    {
-        if (!daemonException && std::uncaught_exceptions()) {
-            handle.markBad();
-            debug("closing daemon connection because of an exception");
-        }
+    if (!daemonException && std::uncaught_exceptions()) {
+        handle.markBad();
+        debug("closing daemon connection because of an exception");
     }
+}
 
-    RemoteStore::Connection * operator -> () { return &*handle; }
-    RemoteStore::Connection & operator * () { return *handle; }
-
-    void processStderr(Sink * sink = 0, Source * source = 0, bool flush = true)
-    {
-        auto ex = handle->processStderr(sink, source, flush);
-        if (ex) {
-            daemonException = true;
-            std::rethrow_exception(ex);
-        }
+void RemoteStore::ConnectionHandle::processStderr(Sink * sink, Source * source, bool flush)
+{
+    auto ex = handle->processStderr(sink, source, flush);
+    if (ex) {
+        daemonException = true;
+        std::rethrow_exception(ex);
     }
-
-    void withFramedSink(std::function<void(Sink & sink)> fun);
-};
+}
 
 
-ConnectionHandle RemoteStore::getConnection()
+RemoteStore::ConnectionHandle RemoteStore::getConnection()
 {
     return ConnectionHandle(connections->get());
 }
@@ -846,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());
@@ -1099,7 +1066,7 @@ std::exception_ptr RemoteStore::Connection::processStderr(Sink * sink, Source *
     return nullptr;
 }
 
-void ConnectionHandle::withFramedSink(std::function<void(Sink & sink)> fun)
+void RemoteStore::ConnectionHandle::withFramedSink(std::function<void(Sink & sink)> fun)
 {
     (*this)->to.flush();
 
diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh
index b12f5437f..a1ae82a0f 100644
--- a/src/libstore/remote-store.hh
+++ b/src/libstore/remote-store.hh
@@ -17,7 +17,6 @@ class Pid;
 struct FdSink;
 struct FdSource;
 template<typename T> class Pool;
-struct ConnectionHandle;
 
 struct RemoteStoreConfig : virtual StoreConfig
 {
@@ -127,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;
@@ -182,6 +179,8 @@ protected:
 
     void setOptions() override;
 
+    struct ConnectionHandle;
+
     ConnectionHandle getConnection();
 
     friend struct ConnectionHandle;
@@ -199,5 +198,4 @@ private:
         std::shared_ptr<Store> evalStore);
 };
 
-
 }
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/store-api.hh b/src/libstore/store-api.hh
index 7b412d2dd..3758c730f 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -99,6 +99,8 @@ typedef std::map<StorePath, std::optional<ContentAddress>> StorePathCAMap;
 
 struct StoreConfig : public Config
 {
+    typedef std::map<std::string, std::string> Params;
+
     using Config::Config;
 
     StoreConfig() = delete;
@@ -153,10 +155,6 @@ struct StoreConfig : public Config
 
 class Store : public std::enable_shared_from_this<Store>, public virtual StoreConfig
 {
-public:
-
-    typedef std::map<std::string, std::string> Params;
-
 protected:
 
     struct PathInfoCacheValue {
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 2bd6517fa..cdb28a001 100644
--- a/src/libstore/uds-remote-store.hh
+++ b/src/libstore/uds-remote-store.hh
@@ -3,13 +3,13 @@
 
 #include "remote-store.hh"
 #include "remote-store-connection.hh"
-#include "local-fs-store.hh"
+#include "indirect-root-store.hh"
 
 namespace nix {
 
 struct UDSRemoteStoreConfig : virtual LocalFSStoreConfig, virtual RemoteStoreConfig
 {
-    UDSRemoteStoreConfig(const Store::Params & params)
+    UDSRemoteStoreConfig(const Params & params)
         : StoreConfig(params)
         , LocalFSStoreConfig(params)
         , RemoteStoreConfig(params)
@@ -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