nix icon indicating copy to clipboard operation
nix copied to clipboard

nix commands shouldn't cache remote git refs

Open ruro opened this issue 2 years ago • 6 comments

Describe the bug

In some situations, nix appears to cache the revision information for remote git repositories when it (arguably) shouldn't.

Steps To Reproduce

  1. Create a flake repository, push the initial commit.
  2. Run any of the following commands:
    • nix flake prefetch 'github:repo/name'
    • nix flake metadata 'github:repo/name'
    • nix build 'github:repo/name'
    • nix run 'github:repo/name'
  3. Make some changes in the repository, commit and push them
  4. Run any of the commands in (2) again.
  5. Observe, that all the commands use the old commit revision instead of checking if the master HEAD was updated.
  6. Run these commands with -vvvv and note the following lines:
    using cache entry '{"name":"source","type":"file","url":"https://api.github.com/repos/repo/name/commits/HEAD"}' -> '{"etag":"W/\"<redacted>\"","url":"https://api.github.com/repos/repo/name/commits/HEAD"}', '/nix/store/<redacted>-source'
    HEAD revision for 'https://api.github.com/repos/repo/name/commits/HEAD' is <old-commit-sha>
    

Expected behavior

nix commands should not cache the ref revision information.

nix-env --version output

nix-env (Nix) 2.8.1

P.S. This is not a GitHub-specific issue. The same problem happens with gitlab:repo/name and 'git+https://github.com/repo/name'.

P.P.S. Turns out, this is not HEAD or master specific either. github:repo/name?ref=other and git+https://github.com/repo/name?ref=other are also apparently cached.

P.P.P.S. The caching seems to expire on its own after some time which makes the current behaviour extra confusing.

ruro avatar Jul 22 '22 17:07 ruro

Yes, these things are cached (until the time specified by the – arguably ill-named tarball-ttl option which defaults to 1 hour).

That's occasionally confusing indeed, but also very useful for speed purposes in some cases (like actually nix run github:foo/bar. If I run it twice in a row, it's extremely convenient to have it just reuse the cached result the second time rather than having to wait for it to contact github to make sure that the latest revision hasn't changed).

thufschmitt avatar Aug 09 '22 09:08 thufschmitt

Yeah, I have already figured it out, but I still think, that it's a UX disaster in some cases. Consider the following:

  1. A user might commit and push to their own repository and then expect nix run github:my/repo to run the updated version.
  2. Yes, the option is poorly named and so is harder to discover in the documentation. Also, there is an expectation, that tarballs and existing revisions won't change, so this cache is "reasonable", but the master of a git repository might be frequently updated, so caching it isn't very intuitive IMHO (but I understand why it was done from the point of performance).
  3. This cache is per-user. So nix run github:foo/bar and sudo nix run github:foo/bar might have cached different revisions.

I propose

  1. Either change the behaviour of nix flake prefetch or add a separate command that fetches the remote despite the cache
  2. Better document the fact that the git remotes are cached.
  3. Maybe have separate options for caching "long living" objects like tarballs/VCS checkouts, and for "frequently updating" objects like VCS refs?
  4. Would it be possible to cache this stuff in the nix store instead of per-user?

ruro avatar Aug 09 '22 14:08 ruro

there is an expectation, that tarballs and existing revisions won't change, so this cache is "reasonable", but the master of a git repository might be frequently updated, so caching it isn't very intuitive IMHO

That's right. There's a bad faith argument to be made that since you're running over the network and most of these stuffs are just eventually consistent, the caching doesn't fundamentally change anything (but I don't really buy it, since in general the “eventually” in “eventually consistent” is within a matter of seconds and not hours). More realistically, this is indeed pretty nifty from a performance point of view, so it would be hard to get rid of it.

Either change the behaviour of nix flake prefetch or add a separate command that fetches the remote despite the cache

Yes, having flake prefetch ignore the cache could be quite sensible (nix flake lock already does that). PRs welcome :wink:

Better document the fact that the git remotes are cached.

That sounds quite reasonable, although I've no idea where to put that

Maybe have separate options for caching "long living" objects like tarballs/VCS checkouts, and for "frequently updating" objects like VCS refs?

Mh could be… although tarballs aren't necessarily more “long living” than VCS refs. To take a stupid example, https://github.com/NixOS/nix/archive/refs/heads/master.tar.gz has the same lifetime as the underlying git ref :)

That being said, renaming the option would make a lot of sense at the very least

Would it be possible to cache this stuff in the nix store instead of per-user?

Not easily. This kind of caching doesn't really match with the store model – Esp. the Git one since it keeps a mutable git clone behind the scenes.

thufschmitt avatar Aug 10 '22 15:08 thufschmitt

Maybe have separate options for caching "long living" objects like tarballs/VCS checkouts, and for "frequently updating" objects like VCS refs?

I might have misunderstood your suggestion, but stuff that's known to be immutable (like fully locked VCS revisions) is actually cached forever. The expiring cache only applies to potentially mutable stuff

thufschmitt avatar Aug 10 '22 15:08 thufschmitt

You can set tarball-ttl = 0 in your nix.conf if you don't want caching. But I don't think that should be the default, since it would make almost every Nix command (e.g. nix run nixpkgs#hello) poll GitHub.

edolstra avatar Aug 10 '22 15:08 edolstra

Would it be possible to cache this stuff in the nix store instead of per-user?

Not easily. This kind of caching doesn't really match with the store model – Esp. the Git one since it keeps a mutable git clone behind the scenes.

I was under the impression that builtins.fetchStuff already cached their results in the store. For example, (builtins.fetchurl "https://nixos.org/") gives /nix/store/5kzh5xvpgfzjmcgc5958x5s4a5cbnix6-nixos.org, but I am guessing that the store only caches the content-addressable result, not the lookup procedure itself. I guess, that the problem with caching things in the nix store is

  1. finding the previously cached result the next time
  2. invalidating the cache after TTL (so you have to remember when was the cache created, but including the time of creation makes the cache no longer content-addressable?)
  3. reusing old cache when re-validating (so you don't have to do a full clone/download each time the TTL runs out)

Did I miss anything?

Maybe have separate options for caching "long living" objects like tarballs/VCS checkouts, and for "frequently updating" objects like VCS refs?

I might have misunderstood your suggestion, but stuff that's known to be immutable (like fully locked VCS revisions) is actually cached forever. The expiring cache only applies to potentially mutable stuff

I see. I was under the impression, that it does a fetch every time the cache is invalidated (which also downloads the corresponding blobs). But I now understand, that this fetch is performed in an already initialized repo, so it's not as bad.

You can set tarball-ttl = 0 in your nix.conf if you don't want caching. But I don't think that should be the default, since it would make almost every Nix command (e.g. nix run nixpkgs#hello) poll GitHub.

Yes, of course. I understand, that completely disabling caching would be bad. Still, I think, that we could provide a friendlier UX here. Maybe by having separate ttl options for different caches. At the very least, something like nix flake prefetch --update would be very useful, because currently you basically have to manually remove the cache if you want the newer version. tarball-ttl = 0 is way too aggressive. Even if you use it in a single command via --option instead of setting it in nix.conf it still invalidates the cached flake-registry.json.

ruro avatar Aug 10 '22 16:08 ruro

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

https://discourse.nixos.org/t/flakes-and-remote-repos-not-being-updated/21580/16

nixos-discourse avatar Sep 14 '22 19:09 nixos-discourse

I stumbled over this exact issue after I committed changes to a branch but they would not be used by a call to nixos-rebuild. I now set the tarball-ttl to 0 as a workaround.

The peculiar thing at my case is: The git repo is local, no network traffic needed. I generally applaud caching for networked stuff (but maybe not if you request a flake directly from cmdline?), but IMO local paths should always be checked for changes.

For reference: The flake URI I used was git+file:///etc/nixos?ref=main#[hostname]

Mynacol avatar Jul 04 '23 17:07 Mynacol

This is very related to #4007 but not quite the same complaint I think.

I looked in the code and I think the solution is to not use the cache in the getRevFromRef functions

elikoga avatar Sep 08 '23 09:09 elikoga

We also need to look at https://github.com/nixos/nix/blob/master/src/libfetchers/git.cc#L500-L502

elikoga avatar Sep 08 '23 10:09 elikoga

By the way, revisiting the problem that the cache is currently per-user: I now understand why caching in /nix/store/ wouldn't work, but this doesn't necessarily mean that the cache has to be per-user. I still think that all the caching should be done by the daemon, not by individual users.

The fact that nix run ... and sudo nix run ... use different caches is still a major UX disaster IMHO.

ruro avatar Sep 08 '23 10:09 ruro

I'm thinking that different download calls should have different (or 0) tarball ttl. The github api calls for example go through the cache too, but I don't see how tarballTtl logic applies here

elikoga avatar Sep 08 '23 10:09 elikoga

The fact that nix run ... and sudo nix run ... use different caches is still a major UX disaster IMHO.

Respectfully disagree. Different users might (and should!) have different access settings for private git repositories. If one could steal private git repositories through the nix cache that would be a security disaster.

ivan-tkatchev avatar Sep 08 '23 10:09 ivan-tkatchev

the problem that the cache is currently per-user

not a problem imo

elikoga avatar Sep 08 '23 12:09 elikoga

The fact that nix run ... and sudo nix run ... use different caches is still a major UX disaster IMHO.

Respectfully disagree. Different users might (and should!) have different access settings for private git repositories. If one could steal private git repositories through the nix cache that would be a security disaster.

The source repository contents are already copied to the store in the current model, so I don't quite understand what your proposed vector of attack is. Admittedly, this (might) be somewhat mitigated by lazy trees, but I think that you really shouldn't rely on lazy trees ability to sometimes elide source copying for your security model.

I think that both per-user and global caching are potentially problematic in a multi-user environment. Using separate user caches leads to the problem where it's basically impossible to verify the flakes' behaviour before running it as root. If you do nix ... and then sudo nix ..., then these 2 commands will use different caches. If you set tarball-ttl = 0 on both commands, then you are susceptible to a race condition.

ruro avatar Sep 08 '23 17:09 ruro

The source repository contents are already copied to the store in the current model

Which store? If you mean 'daemon', then it might not be running on the system or be available to the user. (Users might have private stores.)

Logically it would make sense for the cache to be tied to the store currently in use.

ivan-tkatchev avatar Sep 08 '23 17:09 ivan-tkatchev

Which store? If you mean 'daemon', then it might not be running on the system or be available to the user. (Users might have private stores.)

I mean the nix store that would be used by the nix daemon. I'll admit that I am not too familiar with all the details of how single-user nix, rootless nix and $NIX_STORE_DIR all interact, so there might be some nuance here that I am not considering.

Logically it would make sense for the cache to be tied to the store currently in use.

Yes, I think that the cache SHOULD be tied to the store that is currently in use. Just to be clear, currently the following happens in multi-user (think NixOS) nix when you run nix run git+https://somerepo:

  1. The nix CLI frontend (the process that the user started with nix run ...) tries to figure out what git+https://somerepo is. To do that, it consults (and possibly populates) the nix evaluation cache located at ~/.cache/nix. At the end of this step, nix has obtained a bare clone of the git+https://somerepo repository and stored it somewhere in the cache directory OF THE CURRENT USER.

  2. The nix CLI frontend then asks the nix daemon (which is running as a privileged user) to build this derivation. This consists of the following steps:

    1. prepare for the evaluation (do a copy/checkout of somerepo from ~/.cache/nix to /nix/store)

      • the copy might (?) be elided with lazy trees, but this is a performance optimization, not a security feature
    2. evaluate the flake (produce the .drv file in /nix/store)

      • this might recurse if there are any IFDs
    3. build the derivation (producing the output in /nix/store)

  3. The nix CLI frontend runs the produced output.

The above explanation might be wrong, but this is my current understanding of how multi-user nix works. Feel free to correct me, if I got anything wrong.

Note, that the evaluation cache is per-user ~/.cache/nix while the nix daemon and the nix store are global /nix/store, /var/nix, etc. All I am proposing is that the evaluation cache should also be global in this case (/var/cache/nix perhaps?).

ruro avatar Sep 08 '23 18:09 ruro

You can set tarball-ttl = 0 in your nix.conf if you don't want caching. But I don't think that should be the default, since it would make almost every Nix command (e.g. nix run nixpkgs#hello) poll GitHub.

doesn't almost every nix command already poll github, for the flake registry?

sersorrel avatar Jan 20 '24 18:01 sersorrel

You can set tarball-ttl = 0 in your nix.conf if you don't want caching. But I don't think that should be the default, since it would make almost every Nix command (e.g. nix run nixpkgs#hello) poll GitHub.

doesn't almost every nix command already poll github, for the flake registry?

It does, modulo the fact that the result of the fetch is cached (for the duration of tarball-ttl), so it only happens once in a while

thufschmitt avatar Jan 22 '24 10:01 thufschmitt

My main complaint is that tarball-ttl is used to cache requests that are definetly not tarballs

(like git hoster api requests)

elikoga avatar Jan 22 '24 19:01 elikoga