Add a way to recursively GC paths #8417

balsoft · 2023-05-30T08:07:36Z

Motivation

Refactor GCOptions to make the types reflect possible GC operations, add a way to delete a specific path recursively, and add a --recursive flag to nix store delete.

Context

Fixes #7239

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
documentation in the internal API docs
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

edolstra · 2023-05-30T09:56:24Z

Can't this be done using nix-store --delete $(nix-store -qR <path>)?

balsoft · 2023-05-30T09:57:30Z

Can't this be done using nix-store --delete $(nix-store -qR )?

AFAIU in that case the entire operation will fail if at least one path in the closure is still alive. This PR adds a way to remote all dead paths in a closure. I should clarify that in the documentation.

thufschmitt

I didn't go through the whole of it yet, but from a first pass: I like this very much. It doesn't seem to exactly do what it says though (se the inline comment in gc.cc). Having a solid set of testcases for it would help clarifying that

thufschmitt · 2023-05-31T08:49:08Z

src/libstore/daemon.cc

+        int action;
+        bool ignoreLiveness;
+        StorePathSet pathsToDelete;
+        bool recursive {false};
+        action = readInt(from);
+        pathsToDelete = WorkerProto<StorePathSet>::read(*store, from);
+        from >> ignoreLiveness >> options.maxFreed;
        // obsolete fields
        readInt(from);
        readInt(from);
        readInt(from);
+        if (GET_PROTOCOL_MINOR(clientVersion) >= 31) {
+            from >> recursive;
+        };
+
+        switch (action) {
+            case 0:
+                options.action = GCReturn::Live; break;
+            case 1:
+                options.action = GCReturn::Dead; break;
+            case 2:
+                options.action = GCDelete { .ignoreLiveness = ignoreLiveness }; break;
+            case 3:
+                options.action = GCDelete { .pathsToDelete = GCPathsToDelete { .paths = pathsToDelete, .recursive = recursive }, .ignoreLiveness = ignoreLiveness, }; break;
+        };


Could this be factored out in a WorkerProto<GCOptions>::read method or similar?

thufschmitt · 2023-05-31T08:56:27Z

src/libstore/gc.cc

+    std::visit(overloaded{[&](GCDelete del){
+        if (del.pathsToDelete.has_value() && del.ignoreLiveness) {
+            gcKeepOutputs = false;
+            gcKeepDerivations = false;
+        }
+    }, [](GCReturn arg){}}, options.action);


Suggested change

std::visit(overloaded{[&](GCDelete del){

if (del.pathsToDelete.has_value() && del.ignoreLiveness) {

gcKeepOutputs = false;

gcKeepDerivations = false;

}

}, [](GCReturn arg){}}, options.action);

if (auto del = std::get_if<GCDelete>(&options.action)

&& del->pathsToDelete.has_value() && del->ignoreLiveness) {

gcKeepOutputs = false;

gcKeepDerivations = false;

}

(just a suggestion, but I find it easier to read since the second branch is empty. Also same in a couple places below)

I do like std::visit for completeness checking. Especially with code that is overall quite tricky / high stakes.

On the other hand, I would put each function on its own "lines" so the "match arms" are easy to read as such.

thufschmitt · 2023-06-01T08:50:36Z

src/libstore/gc-store.hh

+struct GCPathsToDelete {
+   StorePathSet paths;
+   bool recursive;
+};

-    GCAction action{gcDeleteDead};
+/* Delete either a given set of paths, or all dead paths */
+struct GCDelete {
+   /* Delete this set, or all dead paths if it is std::nullopt */
+   std::optional<GCPathsToDelete> pathsToDelete;


(nit) I'm not sure that this encoding is really optimal. Shoudln't we rather have pathsToDelete be a 3-way variant (<DeleteAll, DeletePath(StorePath), DeleteClosure(StorePath)>)? That's isomorphic but it feels more natural to me

thufschmitt · 2023-06-01T08:58:47Z

src/libstore/gc.cc

+            if (ret == GCReturn::Live) {
+                for (auto & i : alive)
+                    results.paths.insert(printStorePath(i));
+                return;
+            }

-        printInfo("note: currently hard linking saves %.2f MiB",
-            ((unsharedSize - actualSize - overhead) / (1024.0 * 1024.0)));
-    }
+            if (ret == GCReturn::Dead) {
+                for (auto & i : dead)
+                    results.paths.insert(printStorePath(i));
+                return;


(nit) maybe use a switch instead?

thufschmitt · 2023-06-01T09:05:43Z

src/libstore/gc.cc

+    std::visit(overloaded{
+        [&](GCDelete del){
+            if (del.pathsToDelete.has_value()) {
+                for (auto & i : del.pathsToDelete->paths) {
+                    deleteReferrersClosure(i);
+                    if (!del.pathsToDelete->recursive && !dead.count(i))
+                        throw Error(
+                            "Cannot delete path '%1%' since it is still alive. "
+                            "To find out why, use: "
+                            "nix-store --query --roots",
+                            printStorePath(i));
+                }
+            } if (options.maxFreed > 0) {


I don't think this does what it should:

Even if recursive is set, it won't recurse into the (references) closure of the path. All it will do is not fail if the path can't be deleted

It seems (both from the code and from testing) that it will always trigger a full GC, even when a partial one (closure or single path) is requested

It seems (both from the code and from testing) that it will always trigger a full GC, even when a partial one (closure or single path) is requested

Sorry, that was an off-by-one bug, fixed now.

Even if recursive is set, it won't recurse into the (references) closure of the path. All it will do is not fail if the path can't be deleted

Whoops! I lost the bit of code I wrote to GC recursively...

thufschmitt

I didn't go through the whole of it yet, but from a first pass: I like this very much. It doesn't seem to exactly do what it says though (se the inline comment in gc.cc). Having a solid set of testcases for it would help clarifying that

nixos-discourse · 2023-06-12T13:14:32Z

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

https://discourse.nixos.org/t/tweag-nix-dev-update-49/29047/1

Ericson2314 · 2023-06-14T19:15:17Z

src/libstore/gc-store.hh


-    GCAction action{gcDeleteDead};
+/* Delete either a given set of paths, or all dead paths */


Please continue to use /** style comments so it renders in the internal API docs.

thufschmitt · 2023-06-29T05:35:39Z

Still didn't manage to review this (sorry about that, I've blocked an hour tomorrow afternoon for it), but one thing I can say any way is that it will require tests and documentation

nixos-discourse · 2023-06-29T13:37:18Z

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

https://discourse.nixos.org/t/tweag-nix-dev-update-50/29793/1

blaggacao · 2023-06-29T13:44:35Z

Just to reassure myself: this covers the use case of release version pinning while using the nix store as backend, right?

(By sort of inverting the logic)

domenkozar · 2023-07-21T09:25:46Z

Thanks a lot for doing this!

domenkozar · 2023-07-25T10:03:48Z

@balsoft any chance to rebase and squash?

balsoft · 2023-07-27T09:36:04Z

@domenkozar thanks, done

thufschmitt

Left a few minor comments, but my main issue right now is that it doesn't seem to work 🙃

I've pushed an extended version of the test which shows the issue, let me know if I misunderstood something about how it's supposed to work

thufschmitt · 2023-08-02T08:47:01Z

src/libstore/gc.cc

@@ -375,7 +377,7 @@ void LocalStore::findRuntimeRoots(Roots & roots, bool censor)
        auto storePathRegex = std::regex(quoteRegexChars(storeDir) + R"(/[0-9a-z]+[0-9a-zA-Z\+\-\._\?=]*)");
        while (errno = 0, ent = readdir(procDir.get())) {
            checkInterrupt();
-            if (std::regex_match(ent->d_name, digitsRegex)) {
+            if (std::regex_match(ent->d_name, digitsRegex) && strcmp(pids.c_str(), ent->d_name)) {


What's the reason for this?

thufschmitt · 2023-08-02T09:00:05Z

src/libstore/gc.cc

+                            "nix-store --query --roots",
+                            printStorePath(i));
+                }
+            } else if (options.maxFreed > 0) {


This means that maxFreed will be ignored unless we gc the whole store, right? Worth opening a follow-up issue about it then because that's probably not what we want if we use --recursive

nixos-discourse · 2023-08-09T05:14:26Z

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

https://discourse.nixos.org/t/2023-08-04-nix-team-meeting-minutes-77/31487/1

nyabinary · 2023-08-27T13:53:42Z

Whats the status for this?

domenkozar · 2023-11-15T11:46:16Z

@balsoft this PR doesn't work yet, do you plan to work on it?

balsoft · 2023-11-15T19:33:57Z

Yep. I'm planning to finish the work at some point, definitely.

domenkozar · 2024-01-11T11:42:58Z

@balsoft still the plan?

balsoft · 2024-01-11T15:24:49Z

Yep! Currently tied up on #9287 , once that's done-ish I'll get back to other PRs including this one!

Make `nix store gc` accept installable arguments. If these are provided, then the gc will only happen within the closure of these installables and ignore any path outside, even if it's dead. `nix store gc foo` is morally equivalent to ```bash for validPath in $(nix path-info --recursive foo); do nix store delete "$validPath" || true done ```

Fallback to a full GC if the daemon doesn't support partial ones

thufschmitt · 2024-03-01T16:49:19Z

I've rewritten it a bit to make it less invasive (kept the original changes under https://github.com/tweag/nix/commits/gc-closure_ because the cleanup of GCOptions is rather nice and it would be good to come back to it later).

I've also changed the interface to use nix store gc <installable> instead as that felt more coherent

nixos-discourse · 2024-03-08T08:55:55Z

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

https://discourse.nixos.org/t/tweag-nix-dev-update-55/40996/1

balsoft · 2024-03-22T13:08:51Z

src/libstore/remote-store.cc

+            options.action != GCOptions::gcDeleteSpecific &&
+            ! options.pathsToDelete.empty() &&
+            GET_PROTOCOL_MINOR(conn->daemonVersion) < 38) {
+        warn("Your daemon version is too old to support garbage collecting a closure, falling back to a full gc");


I believe this should be a no-op, doing a GC when I tried to delete just one path can be surprising, and lead to unexpected data loss, especially given that this command is likely to be called in some weird circumstances.

domenkozar · 2024-04-29T15:11:42Z

I've found a bug:

❯ nix store gc /nix/store/w53sjb6pmzdqfcflh92rlmjfzz6xbc33-devenv-shell-env
error:
       error: unexpected argument '/nix/store/w53sjb6pmzdqfcflh92rlmjfzz6xbc33-devenv-shell-env'
Try '/nix/store/a0i9g29xgmk42gjjglzd6k0kzyjkay3n-nix-2.21-devenv/bin/nix --help' for more information.

Where '/nix/store/w53sjb6pmzdqfcflh92rlmjfzz6xbc33-devenv-shell-env is a derivation for nix-shell.

edolstra · 2024-04-29T15:41:26Z

I've also changed the interface to use nix store gc instead as that felt more coherent

I would prefer a different command for that, e.g. nix store delete-path .... Apart from being clearer (you're not doing a GC but deleting a specific path), overloading a command to either delete everything or some paths has the problem that shell code like

nix store gc "${paths[@]}"

will do a full GC if the paths array is empty, which is probably not intended.

balsoft requested review from thufschmitt and edolstra as code owners May 30, 2023 08:07

github-actions bot added new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store labels May 30, 2023

balsoft force-pushed the gc-closure branch 3 times, most recently from 6782bb1 to 11d2199 Compare May 30, 2023 08:22

balsoft force-pushed the gc-closure branch from 11d2199 to fd071dc Compare May 30, 2023 09:58

thufschmitt suggested changes Jun 1, 2023

View reviewed changes

balsoft requested a review from thufschmitt June 6, 2023 13:37

Ericson2314 reviewed Jun 14, 2023

View reviewed changes

thufschmitt self-assigned this Jul 3, 2023

mkenigs mentioned this pull request Jul 5, 2023

Delete store paths for environments more selectively flox/flox#130

Open

balsoft force-pushed the gc-closure branch 3 times, most recently from d30ea9e to a1886ea Compare July 27, 2023 09:58

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Jul 27, 2023

thufschmitt suggested changes Aug 2, 2023

View reviewed changes

domenkozar mentioned this pull request Nov 15, 2023

1.0: Rewrite in Python cachix/devenv#745

Closed

7 tasks

thufschmitt removed their assignment Feb 13, 2024

thufschmitt added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Feb 13, 2024

Théophane Hufschmitt added 3 commits March 1, 2024 06:44

Ensure best-effort compatibility with older daemon version

045f0fc

Fallback to a full GC if the daemon doesn't support partial ones

Add a release note for the closure GC

8db988f

thufschmitt force-pushed the gc-closure branch 2 times, most recently from a3b08e1 to 8db988f Compare March 1, 2024 16:45

github-actions bot added the documentation label Mar 1, 2024

thufschmitt requested review from Ericson2314 and thufschmitt March 1, 2024 17:44

balsoft commented Mar 22, 2024

View reviewed changes

domenkozar mentioned this pull request Apr 29, 2024

Cannot run devenv gc cachix/devenv#1145

Open

edolstra requested a review from fricklerhandwerk as a code owner November 12, 2024 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way to recursively GC paths #8417

Add a way to recursively GC paths #8417

balsoft commented May 30, 2023

edolstra commented May 30, 2023

balsoft commented May 30, 2023

thufschmitt left a comment

thufschmitt May 31, 2023

thufschmitt May 31, 2023

Ericson2314 Jun 14, 2023 •

edited

Loading

Ericson2314 Jun 23, 2023 •

edited

Loading

thufschmitt Jun 1, 2023

thufschmitt Jun 1, 2023

thufschmitt Jun 1, 2023

balsoft Jun 1, 2023

thufschmitt left a comment

nixos-discourse commented Jun 12, 2023

Ericson2314 Jun 14, 2023

thufschmitt commented Jun 29, 2023

nixos-discourse commented Jun 29, 2023

blaggacao commented Jun 29, 2023 •

edited

Loading

domenkozar commented Jul 21, 2023

domenkozar commented Jul 25, 2023

balsoft commented Jul 27, 2023

thufschmitt left a comment

thufschmitt Aug 2, 2023

thufschmitt Aug 2, 2023

nixos-discourse commented Aug 9, 2023

nyabinary commented Aug 27, 2023

domenkozar commented Nov 15, 2023

balsoft commented Nov 15, 2023

domenkozar commented Jan 11, 2024

balsoft commented Jan 11, 2024

thufschmitt commented Mar 1, 2024

nixos-discourse commented Mar 8, 2024

balsoft Mar 22, 2024

domenkozar commented Apr 29, 2024

edolstra commented Apr 29, 2024


		GCAction action{gcDeleteDead};
		/* Delete either a given set of paths, or all dead paths */

Add a way to recursively GC paths #8417

Are you sure you want to change the base?

Add a way to recursively GC paths #8417

Conversation

balsoft commented May 30, 2023

Motivation

Context

Checklist for maintainers

Priorities

edolstra commented May 30, 2023

balsoft commented May 30, 2023

thufschmitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Ericson2314 Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thufschmitt left a comment

Choose a reason for hiding this comment

nixos-discourse commented Jun 12, 2023

Choose a reason for hiding this comment

thufschmitt commented Jun 29, 2023

nixos-discourse commented Jun 29, 2023

blaggacao commented Jun 29, 2023 • edited Loading

domenkozar commented Jul 21, 2023

domenkozar commented Jul 25, 2023

balsoft commented Jul 27, 2023

thufschmitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixos-discourse commented Aug 9, 2023

nyabinary commented Aug 27, 2023

domenkozar commented Nov 15, 2023

balsoft commented Nov 15, 2023

domenkozar commented Jan 11, 2024

balsoft commented Jan 11, 2024

thufschmitt commented Mar 1, 2024

nixos-discourse commented Mar 8, 2024

Choose a reason for hiding this comment

domenkozar commented Apr 29, 2024

edolstra commented Apr 29, 2024

Ericson2314 Jun 14, 2023 •

edited

Loading

Ericson2314 Jun 23, 2023 •

edited

Loading

blaggacao commented Jun 29, 2023 •

edited

Loading