Tell user why a given path is live when it can't be deleted #2046

picnoir · 2018-04-05T20:03:42Z

Hello,

This PR changes the error message displayed while trying to remove a path from the store still referenced by a GC root.

$ nix-store --delete /nix/store/73h7ybrxngp9vd22jgixksy8rcidxqp0-firefox-59.0.1/
  error: cannot delete path /nix/store/73h7ybrxngp9vd22jgixksy8rcidxqp0-firefox-59.0.1' since it is still alive

To:

$ nix-store --delete /nix/store/73h7ybrxngp9vd22jgixksy8rcidxqp0-firefox-59.0.1/
  error: cannot delete path '/nix/store/73h7ybrxngp9vd22jgixksy8rcidxqp0-firefox-59.0.1': it is still needed by '/nix/store/xqkhb8hcjj7hns6qphz7zgmnvpk2rlqm-system-path'

This pull request fixes #1810.

Note: this is my first contribution to Nix, I'm not familiar with the code-base yet, I guess you should be extra-cautious while reviewing this.

shlevy

Overall looks good, thank you! Just some small changes.

shlevy · 2018-04-06T07:56:10Z

src/libstore/gc.cc

+                queryReferrers(i, incomingLinks);
+                for (auto & j : incomingLinks)
+                    if(canReachRoot(state, alreadyVisited, i))
+                        gcRootLink = j;


Should we break after this?

shlevy · 2018-04-06T07:56:51Z

src/libstore/gc.cc

+                for (auto & j : incomingLinks)
+                    if(canReachRoot(state, alreadyVisited, i))
+                        gcRootLink = j;
+               throw Error(format("cannot delete path '%1%': it is still needed by '%2%'") % i % gcRootLink);


Should it be possible for gcRootLink to be unassigned here? If yes, we should account for that in the error message. If no, can you add an assert before the throw?

picnoir · 2018-04-06T09:36:54Z

Thanks for the review! I also clarified some assumptions in the comments. Should I squash this commit to the previous one?

…

On Fri, Apr 06, 2018 at 07:57:13AM +0000, Shea Levy wrote: shlevy requested changes on this pull request. Overall looks good, thank you! Just some small changes. > @@ -786,8 +786,18 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) for (auto & i : options.pathsToDelete) { assertStorePath(i); tryToDelete(state, i); - if (state.dead.find(i) == state.dead.end()) - throw Error(format("cannot delete path '%1%' since it is still alive") % i); + if (state.dead.find(i) == state.dead.end()) { + /* We cannot delete the current path. It is needed by a + * derivation that cannot be garbage collected. */ + PathSet incomingLinks; + PathSet alreadyVisited; + Path gcRootLink; + queryReferrers(i, incomingLinks); + for (auto & j : incomingLinks) + if(canReachRoot(state, alreadyVisited, i)) + gcRootLink = j; Should we `break` after this? > @@ -786,8 +786,18 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) for (auto & i : options.pathsToDelete) { assertStorePath(i); tryToDelete(state, i); - if (state.dead.find(i) == state.dead.end()) - throw Error(format("cannot delete path '%1%' since it is still alive") % i); + if (state.dead.find(i) == state.dead.end()) { + /* We cannot delete the current path. It is needed by a + * derivation that cannot be garbage collected. */ + PathSet incomingLinks; + PathSet alreadyVisited; + Path gcRootLink; + queryReferrers(i, incomingLinks); + for (auto & j : incomingLinks) + if(canReachRoot(state, alreadyVisited, i)) + gcRootLink = j; + throw Error(format("cannot delete path '%1%': it is still needed by '%2%'") % i % gcRootLink); Should it be possible for `gcRootLink` to be unassigned here? If yes, we should account for that in the error message. If no, can you add an assert before the `throw`? -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #2046 (review)

shlevy · 2018-04-06T10:17:11Z

Thanks! Yeah, please squash, otherwise looks good!

picnoir · 2018-04-06T10:32:54Z

Alright, done :).

…

On Fri, Apr 06, 2018 at 10:17:13AM +0000, Shea Levy wrote: Thanks! Yeah, please squash, otherwise looks good! -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #2046 (comment)

edolstra · 2018-04-06T11:11:01Z

src/libstore/gc.cc

-                throw Error(format("cannot delete path '%1%' since it is still alive") % i);
+            if (state.dead.find(i) == state.dead.end()) {
+                /* We cannot delete the current path: it is referenced by
+                   at least an intermediate path linked to a GC root.


This assumption is not correct. The path might be a GC root itself, or be a temporary / in-memory root. In either case it does not necessarily have a referrer.

edolstra · 2018-04-06T11:12:09Z

src/libstore/gc.cc

+                /* We are sure this path is linked to a GC root. We should
+                   have found something.*/
+                assertStorePath(gcRootLink);
+                throw Error(format("cannot delete path '%1%': it is still referenced by '%2%'") % i % gcRootLink);


Maybe we should just tell the user about nix-store -q --roots here.

My general preference is rather than having a computer telling a user what to do, if at all possible it should just do that for the user. But perhaps that means just showing the output of --roots on a failure here.

The thing is that the message is incomplete: it shows a single referrer, even though there might be many more. Also, the referrer might be irrelevant: you want to know what roots cause the path to be reachable.

I would say that I can imagine (i.e. remember) all three situations: when a list of roots is the best explanation, when a list of referrers explains everything and the relevance of roots is unclear, and when the best explanation is a randomly selected route from a store path to a root.

[edit] I was talking about implementing the nix-store -q --tree feature without knowing it already exists.

shlevy · 2018-04-09T12:08:26Z

Reassinged to @edolstra due to his feedback

Print a tree showing the links to the garbage roots when a path cannot be removed from nix-store.

picnoir · 2018-04-09T13:57:08Z

Alright, take 2.

This is a suggestion, please do not merge this PR yet.

UI Proposal

As noted by @edolstra , showing a random link to a GC root is clearly not a correct way to show what's happening in case of failure.

I think a good way to show the references would be to use a tree pretty printer. I made a quick prototype to see if it was possible to implement that without writing a lot of code. Looks like it is.

Here's how it looks like when I try to delete my browser for instance:

> NIX_REMOTE=local src/nix-store/nix-store --delete /nix/store/73h7ybrxngp9vd22jgixksy8rcidxqp0-firefox-59.0.1/
finding garbage collector roots...
0 store paths deleted, 0.00 MiB freed
error: cannot delete path '/nix/store/73h7ybrxngp9vd22jgixksy8rcidxqp0-firefox-59.0.1': it's still referrenced by the following GC roots 

/nix/store/73h7ybrxngp9vd22jgixksy8rcidxqp0-firefox-59.0.1
+---/nix/store/c9c9bc3a72hqabys1qdsqmv5k67b242l-system-path
|   +---/nix/store/7cpkkcxmaynr3c3jgd7g2s3l4mvrg9c9-unit-systemd-fsck-.service
|       +---/nix/store/y2vqxsg8f24wsb1dnm24kby0yxwjsc3z-system-units
|           +---/nix/store/wyrp5q0lcax9wyyxfijscrla14630rx5-etc
|               +---/nix/store/abh6vhxqnbn92vz7y8jlipmkrs2zc761-nixos-system-desktop-nix-18.09pre133640.ea145b68a01
+---/nix/store/fmw6hw9gbzikrgp9s9mca6147nif57l3-system-path
|   +---/nix/store/3d7xaf3ah5xq32ygvk0hqsyb7kprgw8k-unit-polkit.service
|   |   +---/nix/store/sqpfd5k1alx0imdss9a27xyj71mfbswg-system-units
|   |       +---/nix/store/q9rniy22fk4qy06ysbpkijw8fzfmksmi-etc
|   |           +---/nix/store/3g0wyqg54c30y64p45fk3y4s2c8l1g90-nixos-system-desktop-nix-18.09pre133640.ea145b68a01
|   +---/nix/store/701bq97lqx5a4x5xksg1dbdgjrr3f853-dbus-1
|   |   +---/nix/store/2w1ib97vf2w01pdncmj2aginrwplsjgx-unit-dbus.service
|   |       +---/nix/store/6hn91n8cmmzbyijmbh9awgph03zq82b9-user-units
|   +---/nix/store/5ab908kx2wk9s2pq5azf7m85xnashqqb-unit-systemd-fsck-.service
+---/nix/store/xqkhb8hcjj7hns6qphz7zgmnvpk2rlqm-system-path
    +---/nix/store/54r4xiz91vb208wha7rc76hvkilvvx2z-dbus-1
        +---/nix/store/k4djq0ayfs1l4ai6n7bgsc7w65yy57cp-unit-dbus.service
        |   +---/nix/store/kaqppvbnris6njp4m9g89v4p6niwlp66-system-units
        |   |   +---/nix/store/n5735i8ix16illpngvvy1y7ln4z3m8yx-etc
        |   |       +---/nix/store/6ra27a34v58wzbh8y1d61hz281w6q2jr-nixos-system-desktop-nix-18.09pre133640.ea145b68a01
        |   +---/nix/store/q61p5ya18r6aba02yrxd24f3bpbahhw6-user-units

It may get messy if the path is referenced in a lot of places.

@edolstra gave a more succinct alternative in the code review comments: printing the command to use (nix-store -q --roots) to start with and let the user investigate the problem.

I don't have a strong opinion on which one to choose (I just wanted to dive in the nix-store GC code, I'm already satisfied).

If we decide to go the tree way, I'll need to make some small adjustments before any review.

Pretty Printer Code Design

I extracted the already existing tree pretty printer from nix-store and moved it in libstore. The pretty printer now also takes a function as parameter which returns the children for a given path.

I still don't have a big picture of the codebase design: I'm not sure this is the best way to accomplish that.

TODO before merging

Decide which UI we wanna implemet (tree or hint).

If we go the tree way:

Implement tree pretty printer.
E2E regression test on nix-store -q --tree.
Check correctness of the --delete tree output. I feel like something is wrong here.
Add something to visually identify the GC roots on the tree.
Document the new tree function and shady parts.

picnoir · 2018-04-16T14:17:03Z

@nh2 @copumpkin @edolstra @7c6f434c

What do you think about this UI?

bobvanderlinden · 2018-05-26T12:06:41Z

I cannot vote on the validity of the output, but a feature like this is very much appreciated. The tree is somewhat verbose, but it would've helped me already in my quest for why a store path was still alive. I didn't know about nix-store -q --roots. I would've liked the error message itself to show me why a certain derivation is alive. Most use-cases where one sees such an error would be to know why the path is alive. From what I can see, this PR will help the UX significantly.

bobvanderlinden · 2018-08-05T08:42:29Z

I ran into this problem again today and came across this issue again. nix-store -q --roots helped me out once again, but I'm currently (apparently) using this PR as documentation for Nix. Note that I'm searching for cannot delete path ... since it is still alive and #171 shows up, which is how I found this PR again. The manual only mentions the error as example output of nix-store --delete (https://nixos.org/nix/manual/#example), but doesn't mention nix-store --query --roots there.

If it's less friction to merge just a mention of nix-store -q --roots when the error shows up, I'd suggesting doing that. I can make a separate PR if motivation for this PR is low.

picnoir · 2018-08-05T21:45:40Z

I'm still waiting for feedback. I'm open to make some modifications on this if needed.

This PR seems to be stalled though, I think opening another one suggesting --roots might be a good idea.

bobvanderlinden · 2018-08-08T19:29:43Z

Alright I made a PR #2338 which basically mentions nix-store --query --roots and nothing more. I personally do like this PR better (or at least its initial intent), but if it's not correct in some cases it might cause confusion. I'm fine with merging this or #2338 either way.

shlevy suggested changes Apr 6, 2018

View reviewed changes

shlevy self-assigned this Apr 6, 2018

shlevy added improvement UX The way in which users interact with Nix. Higher level than UI. labels Apr 6, 2018

picnoir force-pushed the issue-1810 branch from 75ed4a0 to a0b3a67 Compare April 6, 2018 10:31

edolstra reviewed Apr 6, 2018

View reviewed changes

shlevy added the triaged label Apr 9, 2018

shlevy assigned edolstra and unassigned shlevy Apr 9, 2018

nix-store: Explain why nix path cannot be deleted.

a5269fd

Print a tree showing the links to the garbage roots when a path cannot be removed from nix-store.

picnoir force-pushed the issue-1810 branch from a0b3a67 to a5269fd Compare April 9, 2018 13:57

picnoir mentioned this pull request May 25, 2018

Tell user why a given path is live when it can't be deleted (continued from #171) #1810

Open

bobvanderlinden mentioned this pull request Aug 8, 2018

mention nix-store --query --roots when a path cannot be deleted #2338

Merged

picnoir closed this Oct 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tell user why a given path is live when it can't be deleted #2046

Tell user why a given path is live when it can't be deleted #2046

picnoir commented Apr 5, 2018

shlevy left a comment

shlevy Apr 6, 2018

shlevy Apr 6, 2018

picnoir commented Apr 6, 2018 via email

shlevy commented Apr 6, 2018

picnoir commented Apr 6, 2018 via email

edolstra Apr 6, 2018

edolstra Apr 6, 2018 •

edited

copumpkin Apr 6, 2018

edolstra Apr 6, 2018

7c6f434c Apr 6, 2018

picnoir Apr 6, 2018 •

edited

shlevy commented Apr 9, 2018

picnoir commented Apr 9, 2018 •

edited

picnoir commented Apr 16, 2018

bobvanderlinden commented May 26, 2018

bobvanderlinden commented Aug 5, 2018 •

edited

picnoir commented Aug 5, 2018

bobvanderlinden commented Aug 8, 2018

Tell user why a given path is live when it can't be deleted #2046

Tell user why a given path is live when it can't be deleted #2046

Conversation

picnoir commented Apr 5, 2018

shlevy left a comment

Choose a reason for hiding this comment

shlevy Apr 6, 2018

Choose a reason for hiding this comment

shlevy Apr 6, 2018

Choose a reason for hiding this comment

picnoir commented Apr 6, 2018 via email

shlevy commented Apr 6, 2018

picnoir commented Apr 6, 2018 via email

edolstra Apr 6, 2018

Choose a reason for hiding this comment

edolstra Apr 6, 2018 • edited

Choose a reason for hiding this comment

copumpkin Apr 6, 2018

Choose a reason for hiding this comment

edolstra Apr 6, 2018

Choose a reason for hiding this comment

7c6f434c Apr 6, 2018

Choose a reason for hiding this comment

picnoir Apr 6, 2018 • edited

Choose a reason for hiding this comment

shlevy commented Apr 9, 2018

picnoir commented Apr 9, 2018 • edited

UI Proposal

Pretty Printer Code Design

TODO before merging

picnoir commented Apr 16, 2018

bobvanderlinden commented May 26, 2018

bobvanderlinden commented Aug 5, 2018 • edited

picnoir commented Aug 5, 2018

bobvanderlinden commented Aug 8, 2018

edolstra Apr 6, 2018 •

edited

picnoir Apr 6, 2018 •

edited

picnoir commented Apr 9, 2018 •

edited

bobvanderlinden commented Aug 5, 2018 •

edited