Fix fetchToStore caching #14050

edolstra · 2025-09-22T21:29:05Z

Motivation

fetchToStore() caching is broken because it uses the fingerprint of the accessor, but now that the accessor (typically storeFS) is a composite (like MountedSourceAccessor or AllowListSourceAccessor), there is no fingerprint anymore.

So fetchToStore() now uses the new getFingerprint() method to get the specific fingerprint for the subpath. We now also mount inputs on top of storeFS to make their fingerprints visible to fetchToStore().

To prevent future regressions like this, this PR also adds tests to make sure that most paths are cacheable.

Context

Mostly cherry-picked from DetSys Nix and #13225.

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

xokdvium · 2025-09-22T22:24:41Z

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

+struct MountedSourceAccessor : SourceAccessor
+{
+    virtual void mount(CanonPath mountPoint, ref<SourceAccessor> accessor) = 0;
+
+    /**
+     * Return the accessor mounted on `mountPoint`, or `nullptr` if
+     * there is no such mount point.
+     */
+    virtual std::shared_ptr<SourceAccessor> getMount(CanonPath mountPoint) = 0;


This is great. Like this exactly what's needed for in-memory dummy store.

We'll need to use this with care. E.g. the path ${m}/foo != getMount m foo, so if both are observable in the language, that would be a problem. (And they are substantially different in terms of root and path components)

This shouldn't be a problem here since we don't have lazy trees yet. I.e. identical inputs are deterministically mounted on the same store paths.

xokdvium · 2025-09-22T22:26:20Z

src/libexpr/paths.cc

+
+    if (originalInput.getNarHash() && narHash != *originalInput.getNarHash())
+        throw Error(
+            (unsigned int) 102,


Isn't there an enum for this?

Don't think so... There is Worker::failingExitStatus() which should probably be factored out.

I was meaning to clean that up and unify with BuildResult statuses at some point, fwiw.

Ericson2314 · 2025-09-25T03:15:49Z

src/libexpr/paths.cc

+{
+    auto storePath = fetchToStore(fetchSettings, *store, accessor, FetchMode::Copy, input.getName());
+
+    allowPath(storePath); // FIXME: should just whitelist the entire virtual store


I would like this part done sooner rather than later

Ericson2314 · 2025-09-25T04:17:29Z

I took the "easy parts" of this and did some other cleanups and put in #14080.

Ericson2314 · 2025-09-25T04:44:26Z

src/libexpr/eval.cc

-        auto realStoreDir = dirOf(store->toRealPath(StorePath::dummy));
-        if (settings.pureEval || store->storeDir != realStoreDir) {
-            accessor = settings.pureEval ? storeFS : makeUnionSourceAccessor({accessor, storeFS});
-        }
+        accessor = settings.pureEval ? storeFS.cast<SourceAccessor>() : makeUnionSourceAccessor({accessor, storeFS});


I don't think this is quite right --- evidently we need better unit tests to catch this.

#14080 removes redundant branches while keeping the store->storeDir != realStoreDir fix.

No, this change is necessary. Otherwise, if pureEval isn't enabled, rootFS ends up not containing storeFS, so the mounts on storeFS won't be visible in rootFS.

Mic92 · 2025-09-25T07:21:11Z

@Ericson2314 now this one needs a rebase?

With FilteringSourceAccessor, lstat() needs to throw a different exception if the path is inaccessible than if it doesn't exist.

This returns the fingerprint for a specific subpath. This is intended for "composite" accessors like MountedSourceAccessor, where different subdirectories can have different fingerprints.

Ericson2314 · 2025-09-25T15:25:21Z

I'll rebase it

fetchToStore() caching was broken because it uses the fingerprint of the accessor, but now that the accessor (typically storeFS) is a composite (like MountedSourceAccessor or AllowListSourceAccessor), there was no fingerprint anymore. So fetchToStore now uses the new getFingerprint() method to get the specific fingerprint for the subpath.

This is to test the non-functional property that most paths should be cacheable. We've had frequent cases where caching broken but we didn't notice.

Ericson2314 · 2025-09-25T18:02:01Z

src/libexpr/eval.cc

+
+        // Backward compatibility hack: throw an exception if access
+        // to this path is not allowed.
+        if (auto accessor = res.accessor.dynamic_pointer_cast<FilteringSourceAccessor>())
+            accessor->checkAccess(res.path);


This stuff has been on lazy trees for a while I know, but I guess a this point I am skeptical it is still needed? Especially with #14081

No, they're still needed.

Ericson2314 · 2025-09-30T15:19:54Z

So my current plan (as of yesterday) is that after @xokdvium fixes some bugs, I think #14081 will pass tests, and then I hope we can do that, and then I think we will be better prepared to review this. I want avoid a mixture of mounting and filtering in the pure eval case because I think that is hard to reason about.

edolstra · 2025-10-06T09:24:47Z

and then I think we will be better prepared to review this.

Let's please not block pretty important bug fixes on nice-to-have refactorings. Currently the fetchToStore cache is broken on master, which is a pretty major regression.

xokdvium

Looks good. Other nice-to-haves we can improve on in follow-ups

xokdvium · 2025-10-24T00:43:06Z

This seems to have regressed: nix eval --expr "builtins.readDir /nix/store" --impure

nix eval --expr "builtins.readDir /nix/store" --impure
error:
       … while calling the 'readDir' builtin
         at «string»:1:1:
            1| builtins.readDir /nix/store
             | ^

       error: path '/nix/store/' is not in the Nix store

nixos-discourse · 2025-10-24T23:34:11Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-2-32-0-released/70528/9

github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority fetching Networking with the outside (non-Nix) world, input locking labels Sep 22, 2025

edolstra mentioned this pull request Sep 22, 2025

Create a second Store::getFSAccessor for a single store object #14049

Merged

xokdvium reviewed Sep 22, 2025

View reviewed changes

edolstra added this to Nix team Sep 24, 2025

github-project-automation bot moved this to Triage in Nix team Sep 24, 2025

Ericson2314 reviewed Sep 25, 2025

View reviewed changes

Ericson2314 mentioned this pull request Sep 25, 2025

Some storeFS and similar cleanup #14080

Merged

Ericson2314 reviewed Sep 25, 2025

View reviewed changes

xokdvium mentioned this pull request Sep 25, 2025

libfetchers: avoid re-copying substituted inputs #14041

Merged

getchoo mentioned this pull request Sep 25, 2025

nix copies inputs already in the nix store #11228

Closed

edolstra added 4 commits September 25, 2025 11:20

SourceAccessor: Make lstat() virtual

55c7ef9

With FilteringSourceAccessor, lstat() needs to throw a different exception if the path is inaccessible than if it doesn't exist.

Add SourceAccessor::getFingerprint()

28d11c5

This returns the fingerprint for a specific subpath. This is intended for "composite" accessors like MountedSourceAccessor, where different subdirectories can have different fingerprints.

Git fetcher: Make dirty repos with no commits cacheable

3450a72

Path fetcher: Simplify fingerprint computation

ec6d5c7

edolstra added 2 commits September 25, 2025 11:30

Test against uncacheable paths

4b9735b

This is to test the non-functional property that most paths should be cacheable. We've had frequent cases where caching broken but we didn't notice.

Ericson2314 force-pushed the fix-fetch-to-store-caching branch from 7618094 to 4b9735b Compare September 25, 2025 15:30

Ericson2314 reviewed Sep 25, 2025

View reviewed changes

Merge branch 'master' into fix-fetch-to-store-caching

6721ef5

Mic92 requested a review from Ericson2314 September 26, 2025 18:45

xokdvium approved these changes Oct 6, 2025

View reviewed changes

edolstra merged commit 1e70955 into master Oct 6, 2025
32 checks passed

edolstra deleted the fix-fetch-to-store-caching branch October 6, 2025 17:39

github-project-automation bot moved this from Triage to Done in Nix team Oct 6, 2025

xokdvium mentioned this pull request Oct 24, 2025

Source path error improvements #12915

Closed

Uh oh!

Uh oh!

Fix fetchToStore caching #14050

Fix fetchToStore caching #14050

Uh oh!

Conversation

edolstra commented Sep 22, 2025

Motivation

Context

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Sep 25, 2025

Uh oh!

Ericson2314 Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mic92 commented Sep 25, 2025

Uh oh!

Ericson2314 commented Sep 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edolstra commented Oct 6, 2025

Uh oh!

xokdvium left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xokdvium commented Oct 24, 2025

Uh oh!

nixos-discourse commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Ericson2314 Sep 25, 2025 •

edited

Loading

Ericson2314 commented Sep 30, 2025 •

edited

Loading

xokdvium left a comment •

edited

Loading