From 84fe75a12a085c6b4b8d4ac65a048f569de1252b Mon Sep 17 00:00:00 2001
From: Andrew Brooks <andrew.brooks@flightaware.com>
Date: Tue, 6 Sep 2022 17:48:00 -0500
Subject: [PATCH] Keep created temp dirs inside store, but protect from GC

Implements the approach suggested by feedback on PR #6994, where
tempdir paths are created in the store (now with an exclusive lock).

As part of this work, the currently-broken and unused
`createTempDirInStore` function is updated to create an exclusive lock
on the temp directory in the store.

The GC now makes a non-blocking attempt to lock any store directories
that "look like" the temp directories created by this function, and if
it can't acquire one, ignores the directory.
---
 src/libstore/gc.cc          | 12 ++++++++++++
 src/libstore/local-store.cc | 29 +++++++++++++++++++----------
 src/libstore/local-store.hh |  2 +-
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc
index 4c1a82279..6cd7efbc9 100644
--- a/src/libstore/gc.cc
+++ b/src/libstore/gc.cc
@@ -619,6 +619,18 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
         Path path = storeDir + "/" + std::string(baseName);
         Path realPath = realStoreDir + "/" + std::string(baseName);
 
+        /* There may be temp directories in the store that are still in use
+           by another process. We need to be sure that we can acquire an
+           exclusive lock before deleting them. */
+        AutoCloseFD tmpDirFd;
+        if (baseName.rfind("add-", 0) == 0) {
+            tmpDirFd = open(realPath.c_str(), O_RDONLY | O_DIRECTORY);
+            if (tmpDirFd.get() == -1 || !lockFile(tmpDirFd.get(), ltWrite, false)) {
+                debug("skipping locked tempdir '%s'", realPath);
+                return;
+            }
+        }
+
         printInfo("deleting '%1%'", path);
 
         results.paths.insert(path);
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 6abd52683..5ee451da3 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -1382,13 +1382,15 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name
 
     std::unique_ptr<AutoDelete> delTempDir;
     Path tempPath;
+    Path tempDir;
+    AutoCloseFD tempDirFd;
 
     if (!inMemory) {
         /* Drain what we pulled so far, and then keep on pulling */
         StringSource dumpSource { dump };
         ChainSource bothSource { dumpSource, source };
 
-        auto tempDir = createTempDir("", "add");
+        std::tie(tempDir, tempDirFd) = createTempDirInStore();
         delTempDir = std::make_unique<AutoDelete>(tempDir);
         tempPath = tempDir + "/x";
 
@@ -1431,6 +1433,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name
             } else {
                 /* Move the temporary path we restored above. */
                 moveFile(tempPath, realPath);
+                tempDirFd.close();
             }
 
             /* For computing the nar hash. In recursive SHA-256 mode, this
@@ -1507,18 +1510,24 @@ StorePath LocalStore::addTextToStore(
 
 
 /* Create a temporary directory in the store that won't be
-   garbage-collected. */
-Path LocalStore::createTempDirInStore()
+   garbage-collected until the returned FD is closed. */
+std::pair<Path, AutoCloseFD> LocalStore::createTempDirInStore()
 {
-    Path tmpDir;
+    Path tmpDirFn;
+    AutoCloseFD tmpDirFd;
+    bool lockedByUs = false;
     do {
         /* There is a slight possibility that `tmpDir' gets deleted by
-           the GC between createTempDir() and addTempRoot(), so repeat
-           until `tmpDir' exists. */
-        tmpDir = createTempDir(realStoreDir);
-        addTempRoot(parseStorePath(tmpDir));
-    } while (!pathExists(tmpDir));
-    return tmpDir;
+           the GC between createTempDir() and when we acquire a lock on it.
+           We'll repeat until 'tmpDir' exists and we've locked it. */
+        tmpDirFn = createTempDir(realStoreDir, "add");
+        tmpDirFd = open(tmpDirFn.c_str(), O_RDONLY | O_DIRECTORY);
+        if (tmpDirFd.get() < 0) {
+            continue;
+        }
+        lockedByUs = lockFile(tmpDirFd.get(), ltWrite, true);
+    } while (!pathExists(tmpDirFn) || !lockedByUs);
+    return {tmpDirFn, std::move(tmpDirFd)};
 }
 
 
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index 70d225be3..bd0ce1fe6 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -256,7 +256,7 @@ private:
 
     void findRuntimeRoots(Roots & roots, bool censor);
 
-    Path createTempDirInStore();
+    std::pair<Path, AutoCloseFD> createTempDirInStore();
 
     void checkDerivationOutputs(const StorePath & drvPath, const Derivation & drv);