Merge pull request #93 from DeterminateSystems/fix-fetcher-cache

Fix fetchToStore() caching

Author: edolstra

src/libexpr/paths.cc

@@ -80,6 +80,7 @@ StorePath EvalState::mountInput(
     storeFS->mount(CanonPath(store->printStorePath(storePath)), accessor);
 
     if (requireLockable && (!settings.lazyTrees || !input.isLocked()) && !input.getNarHash()) {
+        // FIXME: use fetchToStore to make it cache this
         auto narHash = accessor->hashPath(CanonPath::root);
         input.attrs.insert_or_assign("narHash", narHash.to_string(HashFormat::SRI, true));
     }

src/libfetchers/fetch-to-store.cc

@@ -31,15 +31,16 @@ StorePath fetchToStore(
     // a `PosixSourceAccessor` pointing to a store path.
 
     std::optional<fetchers::Cache::Key> cacheKey;
+    std::optional<std::string> fingerprint;
 
-    if (!filter && path.accessor->fingerprint) {
-        cacheKey = makeFetchToStoreCacheKey(std::string{name}, *path.accessor->fingerprint, method, path.path.abs());
+    if (!filter && (fingerprint = path.accessor->getFingerprint(path.path))) {
+        cacheKey = makeFetchToStoreCacheKey(std::string{name}, *fingerprint, method, path.path.abs());
         if (auto res = fetchers::getCache()->lookupStorePath(*cacheKey, store)) {
             debug("store path cache hit for '%s'", path);
             return res->storePath;
         }
     } else
-        debug("source path '%s' is uncacheable", path);
+        debug("source path '%s' is uncacheable (%d, %d)", path, filter, (bool) fingerprint);
 
     Activity act(*logger, lvlChatty, actUnknown,
         fmt(mode == FetchMode::DryRun ? "hashing '%s'" : "copying '%s' to the store", path));
@@ -55,7 +56,7 @@ StorePath fetchToStore(
 
     debug(mode == FetchMode::DryRun ? "hashed '%s'" : "copied '%s' to '%s'", path, store.printStorePath(storePath));
 
-    if (cacheKey && mode == FetchMode::Copy)
+    if (cacheKey)
         fetchers::getCache()->upsert(*cacheKey, store, {}, storePath);
 
     return storePath;

src/libfetchers/fetchers.cc

@@ -338,7 +338,8 @@ std::pair<ref<SourceAccessor>, Input> Input::getAccessorUnchecked(ref<Store> sto
 
             auto accessor = make_ref<SubstitutedSourceAccessor>(makeStorePathAccessor(store, storePath));
 
-            accessor->fingerprint = getFingerprint(store);
+            if (auto fingerprint = getFingerprint(store))
+                accessor->setFingerprint(*fingerprint);
 
             // FIXME: ideally we would use the `showPath()` of the
             // "real" accessor for this fetcher type.
@@ -352,8 +353,10 @@ std::pair<ref<SourceAccessor>, Input> Input::getAccessorUnchecked(ref<Store> sto
 
     auto [accessor, result] = scheme->getAccessor(store, *this);
 
-    assert(!accessor->fingerprint);
-    accessor->fingerprint = result.getFingerprint(store);
+    assert(!accessor->getFingerprint(CanonPath::root));
+
+    if (auto fingerprint = getFingerprint(store))
+        accessor->setFingerprint(*fingerprint);
 
     return {accessor, std::move(result)};
 }

src/libfetchers/filtering-source-accessor.cc

@@ -14,6 +14,12 @@ std::string FilteringSourceAccessor::readFile(const CanonPath & path)
     return next->readFile(prefix / path);
 }
 
+void FilteringSourceAccessor::readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback)
+{
+    checkAccess(path);
+    return next->readFile(prefix / path, sink, sizeCallback);
+}
+
 bool FilteringSourceAccessor::pathExists(const CanonPath & path)
 {
     return isAllowed(path) && next->pathExists(prefix / path);
@@ -52,6 +58,16 @@ std::string FilteringSourceAccessor::showPath(const CanonPath & path)
     return displayPrefix + next->showPath(prefix / path) + displaySuffix;
 }
 
+std::optional<std::string> FilteringSourceAccessor::getFingerprint(const CanonPath & path)
+{
+    return next->getFingerprint(prefix / path);
+}
+
+void FilteringSourceAccessor::setFingerprint(std::string fingerprint)
+{
+    next->setFingerprint(std::move(fingerprint));
+}
+
 void FilteringSourceAccessor::checkAccess(const CanonPath & path)
 {
     if (!isAllowed(path))

src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh

@@ -36,6 +36,8 @@ struct FilteringSourceAccessor : SourceAccessor
 
     std::string readFile(const CanonPath & path) override;
 
+    void readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback) override;
+
     bool pathExists(const CanonPath & path) override;
 
     Stat lstat(const CanonPath & path) override;
@@ -48,6 +50,10 @@ struct FilteringSourceAccessor : SourceAccessor
 
     std::string showPath(const CanonPath & path) override;
 
+    std::optional<std::string> getFingerprint(const CanonPath & path) override;
+
+    void setFingerprint(std::string fingerprint) override;
+
     /**
      * Call `makeNotAllowedError` to throw a `RestrictedPathError`
      * exception if `isAllowed()` returns `false` for `path`.

src/libutil/include/nix/util/forwarding-source-accessor.hh

@@ -52,6 +52,16 @@ struct ForwardingSourceAccessor : SourceAccessor
     {
         return next->getPhysicalPath(path);
     }
+
+    std::optional<std::string> getFingerprint(const CanonPath & path) override
+    {
+        return next->getFingerprint(path);
+    }
+
+    void setFingerprint(std::string fingerprint) override
+    {
+        next->setFingerprint(std::move(fingerprint));
+    }
 };
 
 }

src/libutil/include/nix/util/source-accessor.hh

@@ -177,10 +177,27 @@ struct SourceAccessor : std::enable_shared_from_this<SourceAccessor>
         SymlinkResolution mode = SymlinkResolution::Full);
 
     /**
-     * A string that uniquely represents the contents of this
-     * accessor. This is used for caching lookups (see `fetchToStore()`).
+     * Return a string that uniquely represents the contents of this
+     * accessor. This is used for caching lookups (see
+     * `fetchToStore()`).
+     *
+     * Fingerprints are generally for the entire accessor, but this
+     * method takes a `path` argument to support accessors like
+     * `MountedSourceAccessor` that combine multiple underlying
+     * accessors. A fingerprint should only be returned if it uniquely
+     * represents everything under `path`.
      */
-    std::optional<std::string> fingerprint;
+    virtual std::optional<std::string> getFingerprint(const CanonPath & path)
+    {
+        return _fingerprint;
+    }
+
+    virtual void setFingerprint(std::string fingerprint)
+    {
+        _fingerprint = std::move(fingerprint);
+    }
+
+    std::optional<std::string> _fingerprint;
 
     /**
      * Return the maximum last-modified time of the files in this

src/libutil/mounted-source-accessor.cc

@@ -90,6 +90,15 @@ struct MountedSourceAccessorImpl : MountedSourceAccessor
         else
             return nullptr;
     }
+
+    std::optional<std::string> getFingerprint(const CanonPath & path) override
+    {
+        auto [accessor, subpath] = resolve(path);
+        // FIXME: check that there are no mounts underneath the mount
+        // point of `accessor`, since that would invalidate the
+        // fingerprint. (However we don't have such at the moment.)
+        return accessor->getFingerprint(subpath);
+    }
 };
 
 ref<MountedSourceAccessor> makeMountedSourceAccessor(std::map<CanonPath, ref<SourceAccessor>> mounts)