From 35c98a59c5f09d8ca376ac809e87d14ff2fcbde1 Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Fri, 8 Oct 2021 16:58:19 +0200
Subject: [PATCH] Fix GC when there are cycles in the referrers graph

(where "referrers" includes the reverse of derivation outputs and
derivers). Now we do a full traversal to look if we can reach any
root. If not, all paths reached can be deleted.
---
 src/libstore/gc.cc          | 74 +++++++++++++++++++++----------------
 src/libstore/local-store.hh |  5 +--
 2 files changed, 45 insertions(+), 34 deletions(-)

diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc
index 969066092..4123a90be 100644
--- a/src/libstore/gc.cc
+++ b/src/libstore/gc.cc
@@ -499,34 +499,29 @@ void LocalStore::deleteFromStore(GCState & state, std::string_view baseName)
 }
 
 
-bool LocalStore::tryToDelete(
+bool LocalStore::canReachRoot(
     GCState & state,
     StorePathSet & visited,
-    const StorePath & path,
-    bool recursive)
+    const StorePath & path)
 {
     checkInterrupt();
 
     //Activity act(*logger, lvlDebug, format("considering whether to delete '%1%'") % path);
 
-    /* Wake up any client waiting for deletion of this path to
-       finish. */
-    Finally releasePending([&]() {
-        auto shared(state.shared.lock());
-        shared->pending.reset();
-        state.wakeup.notify_all();
-    });
+    if (state.options.action == GCOptions::gcDeleteSpecific
+        && !state.options.pathsToDelete.count(path))
+        return true;
 
-    if (!visited.insert(path).second) return true;
+    if (!visited.insert(path).second) return false;
 
-    if (state.alive.count(path)) return false;
+    if (state.alive.count(path)) return true;
 
-    if (state.dead.count(path)) return true;
+    if (state.dead.count(path)) return false;
 
     if (state.roots.count(path)) {
         debug("cannot delete '%s' because it's a root", printStorePath(path));
         state.alive.insert(path);
-        return false;
+        return true;
     }
 
     if (isValidPath(path)) {
@@ -555,12 +550,9 @@ bool LocalStore::tryToDelete(
         }
 
         for (auto & i : incoming)
-            if (i != path
-                && (recursive || state.options.pathsToDelete.count(i))
-                && !tryToDelete(state, visited, i, recursive))
-            {
+            if (i != path && canReachRoot(state, visited, i)) {
                 state.alive.insert(path);
-                return false;
+                return true;
             }
     }
 
@@ -568,18 +560,11 @@ bool LocalStore::tryToDelete(
         auto hashPart = std::string(path.hashPart());
         auto shared(state.shared.lock());
         if (shared->tempRoots.count(hashPart))
-            return false;
+            return true;
         shared->pending = hashPart;
     }
 
-    state.dead.insert(path);
-
-    if (state.shouldDelete) {
-        invalidatePathChecked(path);
-        deleteFromStore(state, path.to_string());
-    }
-
-    return true;
+    return false;
 }
 
 
@@ -628,7 +613,6 @@ void LocalStore::removeUnusedLinks(const GCState & state)
 }
 
 
-
 void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
 {
     GCState state(options, results);
@@ -769,12 +753,21 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
 
         for (auto & i : options.pathsToDelete) {
             StorePathSet visited;
-            if (!tryToDelete(state, visited, i, false))
+
+            if (canReachRoot(state, visited, i))
                 throw Error(
                     "cannot delete path '%1%' since it is still alive. "
                     "To find out why, use: "
                     "nix-store --query --roots",
                     printStorePath(i));
+
+            auto sorted = topoSortPaths(visited);
+            for (auto & path : sorted) {
+                if (state.dead.count(path)) continue;
+                invalidatePathChecked(path);
+                deleteFromStore(state, path.to_string());
+                state.dead.insert(path);
+            }
         }
 
     } else if (options.maxFreed > 0) {
@@ -801,7 +794,26 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
 
                 if (auto storePath = maybeParseStorePath(storeDir + "/" + name)) {
                     StorePathSet visited;
-                    tryToDelete(state, visited, *storePath, true);
+
+                    /* Wake up any GC client waiting for deletion of
+                       the paths in 'visited' to finish. */
+                    Finally releasePending([&]() {
+                        auto shared(state.shared.lock());
+                        shared->pending.reset();
+                        state.wakeup.notify_all();
+                    });
+
+                    if (!canReachRoot(state, visited, *storePath)) {
+                        auto sorted = topoSortPaths(visited);
+                        for (auto & path : sorted) {
+                            if (state.dead.count(path)) continue;
+                            if (state.shouldDelete) {
+                                invalidatePathChecked(path);
+                                deleteFromStore(state, path.to_string());
+                            }
+                            state.dead.insert(path);
+                        }
+                    }
                 } else
                     deleteFromStore(state, name);
 
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index 08bbc1a7d..8629e1528 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -240,11 +240,10 @@ private:
 
     struct GCState;
 
-    bool tryToDelete(
+    bool canReachRoot(
         GCState & state,
         StorePathSet & visited,
-        const StorePath & path,
-        bool recursive);
+        const StorePath & path);
 
     void deleteFromStore(GCState & state, std::string_view baseName);