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

Fix broken fetchToStore() caching

src/libfetchers/fetch-to-store.cc

@@ -31,10 +31,14 @@ StorePath fetchToStore(
     // a `PosixSourceAccessor` pointing to a store path.
 
     std::optional<fetchers::Cache::Key> cacheKey;
-    std::optional<std::string> fingerprint;
 
-    if (!filter && (fingerprint = path.accessor->getFingerprint(path.path))) {
+    auto [subpath, fingerprint] =
-        cacheKey = makeFetchToStoreCacheKey(std::string{name}, *fingerprint, method, path.path.abs());
+        filter
+        ? std::pair<CanonPath, std::optional<std::string>>{path.path, std::nullopt}
+        : path.accessor->getFingerprint(path.path);
+
+    if (fingerprint) {
+        cacheKey = makeFetchToStoreCacheKey(std::string{name}, *fingerprint, method, subpath.abs());
         if (auto res = fetchers::getCache()->lookupStorePath(*cacheKey, store, mode == FetchMode::DryRun)) {
             debug("store path cache hit for '%s'", path);
             return res->storePath;

src/libfetchers/fetchers.cc

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

src/libfetchers/filtering-source-accessor.cc

@@ -58,16 +58,13 @@ std::string FilteringSourceAccessor::showPath(const CanonPath & path)
     return displayPrefix + next->showPath(prefix / path) + displaySuffix;
 }
 
-std::optional<std::string> FilteringSourceAccessor::getFingerprint(const CanonPath & path)
+std::pair<CanonPath, std::optional<std::string>> FilteringSourceAccessor::getFingerprint(const CanonPath & path)
 {
+    if (fingerprint)
+        return {path, fingerprint};
     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

@@ -50,9 +50,7 @@ struct FilteringSourceAccessor : SourceAccessor
 
     std::string showPath(const CanonPath & path) override;
 
-    std::optional<std::string> getFingerprint(const CanonPath & path) override;
+    std::pair<CanonPath, std::optional<std::string>> getFingerprint(const CanonPath & path) override;
-
-    void setFingerprint(std::string fingerprint) override;
 
     /**
      * Call `makeNotAllowedError` to throw a `RestrictedPathError`

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

@@ -52,16 +52,6 @@ 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,28 +177,32 @@ struct SourceAccessor : std::enable_shared_from_this<SourceAccessor>
         SymlinkResolution mode = SymlinkResolution::Full);
 
     /**
-     * Return a string that uniquely represents the contents of this
+     * A string that uniquely represents the contents of this
-     * accessor. This is used for caching lookups (see
+     * accessor. This is used for caching lookups (see `fetchToStore()`).
-     * `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`.
      */
-    virtual std::optional<std::string> getFingerprint(const CanonPath & path)
+    std::optional<std::string> fingerprint;
-    {
-        return _fingerprint;
-    }
 
-    virtual void setFingerprint(std::string fingerprint)
+    /**
+     * Return the fingerprint for `path`. This is usually the
+     * fingerprint of the current accessor, but for composite
+     * accessors (like `MountedSourceAccessor`), we want to return the
+     * fingerprint of the "inner" accessor if the current one lacks a
+     * fingerprint.
+     *
+     * So this method is intended to return the most-outer accessor
+     * that has a fingerprint for `path`. It also returns the path that `path`
+     * corresponds to in that accessor.
+     *
+     * For example: in a `MountedSourceAccessor` that has
+     * `/nix/store/foo` mounted,
+     * `getFingerprint("/nix/store/foo/bar")` will return the path
+     * `/bar` and the fingerprint of the `/nix/store/foo` accessor.
+     */
+    virtual std::pair<CanonPath, std::optional<std::string>> getFingerprint(const CanonPath & path)
     {
-        _fingerprint = std::move(fingerprint);
+        return {path, fingerprint};
     }
 
-    std::optional<std::string> _fingerprint;
-
     /**
      * Return the maximum last-modified time of the files in this
      * tree, if available.

src/libutil/mounted-source-accessor.cc

@@ -91,12 +91,11 @@ struct MountedSourceAccessorImpl : MountedSourceAccessor
             return nullptr;
     }
 
-    std::optional<std::string> getFingerprint(const CanonPath & path) override
+    std::pair<CanonPath, std::optional<std::string>> getFingerprint(const CanonPath & path) override
     {
+        if (fingerprint)
+            return {path, fingerprint};
         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);
     }
 };

tests/functional/flakes/flakes.sh

@@ -112,6 +112,12 @@ nix build -o "$TEST_ROOT/result" "git+file://$flake1Dir#default"
 nix build -o "$TEST_ROOT/result" "$flake1Dir?ref=HEAD#default"
 nix build -o "$TEST_ROOT/result" "git+file://$flake1Dir?ref=HEAD#default"
 
+# Check that the fetcher cache works.
+if [[ $(nix config show lazy-trees) = false ]]; then
+    nix build -o "$TEST_ROOT/result" "git+file://$flake1Dir?ref=HEAD#default" -vvvvv 2>&1 | grepQuietInverse "source path.*is uncacheable"
+    nix build -o "$TEST_ROOT/result" "git+file://$flake1Dir?ref=HEAD#default" -vvvvv 2>&1 | grepQuiet "store path cache hit"
+fi
+
 # Check that relative paths are allowed for git flakes.
 # This may change in the future once git submodule support is refined.
 # See: https://discourse.nixos.org/t/57783 and #9708.