Merge pull request #130 from DeterminateSystems/improve-nix-store-delete-errors

nix store delete: Show why deletion fails

src/libstore/daemon.cc

@@ -730,6 +730,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
         options.action = (GCOptions::GCAction) readInt(conn.from);
         options.pathsToDelete = WorkerProto::Serialise<StorePathSet>::read(*store, rconn);
         conn.from >> options.ignoreLiveness >> options.maxFreed;
+        options.censor = !trusted;
         // obsolete fields
         readInt(conn.from);
         readInt(conn.from);

src/libstore/gc.cc

@@ -208,7 +208,7 @@ void LocalStore::findTempRoots(Roots & tempRoots, bool censor)
         while ((end = contents.find((char) 0, pos)) != std::string::npos) {
             Path root(contents, pos, end - pos);
             debug("got temporary root '%s'", root);
-            tempRoots[parseStorePath(root)].emplace(censor ? censored : fmt("{temp:%d}", pid));
+            tempRoots[parseStorePath(root)].emplace(censor ? censored : fmt("{nix-process:%d}", pid));
             pos = end + 1;
         }
     }
@@ -458,13 +458,14 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
     bool gcKeepOutputs = settings.gcKeepOutputs;
     bool gcKeepDerivations = settings.gcKeepDerivations;
 
-    std::unordered_set<StorePath> roots, dead, alive;
+    Roots roots;
+    std::unordered_set<StorePath> dead, alive;
 
     struct Shared
     {
         // The temp roots only store the hash part to make it easier to
         // ignore suffixes like '.lock', '.chroot' and '.check'.
-        std::unordered_set<std::string> tempRoots;
+        std::unordered_map<std::string, GcRootInfo> tempRoots;
 
         // Hash part of the store path currently being deleted, if
         // any.
@@ -573,7 +574,8 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
                                 debug("got new GC root '%s'", path);
                                 auto hashPart = std::string(storePath->hashPart());
                                 auto shared(_shared.lock());
-                                shared->tempRoots.insert(hashPart);
+                                // FIXME: could get the PID from the socket.
+                                shared->tempRoots.insert_or_assign(hashPart, "{nix-process:unknown}");
                                 /* If this path is currently being
                                    deleted, then we have to wait until
                                    deletion is finished to ensure that
@@ -612,19 +614,18 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
     /* Find the roots.  Since we've grabbed the GC lock, the set of
        permanent roots cannot increase now. */
     printInfo("finding garbage collector roots...");
-    Roots rootMap;
     if (!options.ignoreLiveness)
-        findRootsNoTemp(rootMap, true);
+        findRootsNoTemp(roots, options.censor);
-
-    for (auto & i : rootMap) roots.insert(i.first);
 
     /* Read the temporary roots created before we acquired the global
        GC root. Any new roots will be sent to our socket. */
-    Roots tempRoots;
+    {
-    findTempRoots(tempRoots, true);
+        Roots tempRoots;
-    for (auto & root : tempRoots) {
+        findTempRoots(tempRoots, options.censor);
-        _shared.lock()->tempRoots.insert(std::string(root.first.hashPart()));
+        for (auto & root : tempRoots)
-        roots.insert(root.first);
+            _shared.lock()->tempRoots.insert_or_assign(
+                std::string(root.first.hashPart()),
+                *root.second.begin());
     }
 
     /* Synchronisation point for testing, see tests/functional/gc-non-blocking.sh. */
@@ -716,21 +717,35 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
                 } catch (InvalidPath &) { }
             };
 
+            if (options.action == GCOptions::gcDeleteSpecific
+                && !options.pathsToDelete.count(*path))
+            {
+                throw Error(
+                    "Cannot delete path '%s' because it's referenced by path '%s'.",
+                    printStorePath(start),
+                    printStorePath(*path));
+            }
+
             /* If this is a root, bail out. */
-            if (roots.count(*path)) {
+            if (auto i = roots.find(*path); i != roots.end()) {
+                if (options.action == GCOptions::gcDeleteSpecific)
+                    throw Error(
+                        "Cannot delete path '%s' because it's referenced by the GC root '%s'.",
+                        printStorePath(start),
+                        *i->second.begin());
                 debug("cannot delete '%s' because it's a root", printStorePath(*path));
                 return markAlive();
             }
 
-            if (options.action == GCOptions::gcDeleteSpecific
-                && !options.pathsToDelete.count(*path))
-                return;
-
             {
                 auto hashPart = std::string(path->hashPart());
                 auto shared(_shared.lock());
-                if (shared->tempRoots.count(hashPart)) {
+                if (auto i = shared->tempRoots.find(hashPart); i != shared->tempRoots.end()) {
-                    debug("cannot delete '%s' because it's a temporary root", printStorePath(*path));
+                    if (options.action == GCOptions::gcDeleteSpecific)
+                        throw Error(
+                            "Cannot delete path '%s' because it's in use by '%s'.",
+                            printStorePath(start),
+                            i->second);
                     return markAlive();
                 }
                 shared->pending = hashPart;
@@ -789,12 +804,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
 
         for (auto & i : options.pathsToDelete) {
             deleteReferrersClosure(i);
-            if (!dead.count(i))
+            assert(dead.count(i));
-                throw Error(
-                    "Cannot delete path '%1%' since it is still alive. "
-                    "To find out why, use: "
-                    "nix-store --query --roots and nix-store --query --referrers",
-                    printStorePath(i));
         }
 
     } else if (options.maxFreed > 0) {

src/libstore/include/nix/store/gc-store.hh

@@ -7,8 +7,11 @@
 
 namespace nix {
 
+// FIXME: should turn this into an std::variant to represent the
+// several root types.
+using GcRootInfo = std::string;
 
-typedef std::unordered_map<StorePath, std::unordered_set<std::string>> Roots;
+typedef std::unordered_map<StorePath, std::unordered_set<GcRootInfo>> Roots;
 
 
 struct GCOptions
@@ -53,6 +56,12 @@ struct GCOptions
      * Stop after at least `maxFreed` bytes have been freed.
      */
     uint64_t maxFreed{std::numeric_limits<uint64_t>::max()};
+
+    /**
+     * Whether to hide potentially sensitive information about GC
+     * roots (such as PIDs).
+     */
+    bool censor = false;
 };
 
 

tests/functional/gc-runtime.nix

@@ -9,6 +9,7 @@ mkDerivation {
 
       cat > $out/program <<EOF
       #! ${shell}
+      echo x > \$TEST_ROOT/fifo
       sleep 10000
       EOF
 

tests/functional/gc-runtime.sh

@@ -21,11 +21,16 @@ nix-env -p "$profiles/test" -f ./gc-runtime.nix -i gc-runtime
 outPath=$(nix-env -p "$profiles/test" -q --no-name --out-path gc-runtime)
 echo "$outPath"
 
+fifo="$TEST_ROOT/fifo"
+mkfifo "$fifo"
+
 echo "backgrounding program..."
-"$profiles"/test/program &
+"$profiles"/test/program "$fifo" &
-sleep 2 # hack - wait for the program to get started
 child=$!
 echo PID=$child
+cat "$fifo"
+
+expectStderr 1 nix-store --delete "$outPath" | grepQuiet "Cannot delete path.*because it's referenced by the GC root '/proc/"
 
 nix-env -p "$profiles/test" -e gc-runtime
 nix-env -p "$profiles/test" --delete-generations old

tests/functional/gc.sh

@@ -23,10 +23,10 @@ if nix-store --gc --print-dead | grep -E "$outPath"$; then false; fi
 nix-store --gc --print-dead
 
 inUse=$(readLink "$outPath/reference-to-input-2")
-if nix-store --delete "$inUse"; then false; fi
+expectStderr 1 nix-store --delete "$inUse" | grepQuiet "Cannot delete path.*because it's referenced by path '"
 test -e "$inUse"
 
-if nix-store --delete "$outPath"; then false; fi
+expectStderr 1 nix-store --delete "$outPath" | grepQuiet "Cannot delete path.*because it's referenced by the GC root "
 test -e "$outPath"
 
 for i in "$NIX_STORE_DIR"/*; do

tests/functional/local-overlay-store/delete-refs-inner.sh

@@ -22,14 +22,14 @@ input2=$(nix-build ../hermetic.nix --no-out-link --arg busybox "$busybox" --arg
 input3=$(nix-build ../hermetic.nix --no-out-link --arg busybox "$busybox" --arg withFinalRefs true --arg seed 2 -A passthru.input3 -j0)
 
 # Can't delete because referenced
-expectStderr 1 nix-store --delete $input1 | grepQuiet "Cannot delete path"
+expectStderr 1 nix-store --delete $input1 | grepQuiet "Cannot delete path.*because it's referenced by path"
-expectStderr 1 nix-store --delete $input2 | grepQuiet "Cannot delete path"
+expectStderr 1 nix-store --delete $input2 | grepQuiet "Cannot delete path.*because it's referenced by path"
-expectStderr 1 nix-store --delete $input3 | grepQuiet "Cannot delete path"
+expectStderr 1 nix-store --delete $input3 | grepQuiet "Cannot delete path.*because it's referenced by path"
 
 # These same paths are referenced in the lower layer (by the seed 1
 # build done in `initLowerStore`).
-expectStderr 1 nix-store --store "$storeA" --delete $input2 | grepQuiet "Cannot delete path"
+expectStderr 1 nix-store --store "$storeA" --delete $input2 | grepQuiet "Cannot delete path.*because it's referenced by path"
-expectStderr 1 nix-store --store "$storeA" --delete $input3 | grepQuiet "Cannot delete path"
+expectStderr 1 nix-store --store "$storeA" --delete $input3 | grepQuiet "Cannot delete path.*because it's referenced by path"
 
 # Can delete
 nix-store --delete $hermetic