libstore: always delete PathLocks lockfiles

they are not being created with O_EXCL and their existence is not
checked for using the PathLocks api, so we will boldy assume that
*nothing* checks for lock file existence. deleting lockfiles when
we're done with them in all cases makes the code cleaner, and any
failing builds no longer litter their store with stale lockfiles.

Change-Id: Iffb588d29b00e6aca32fcb6776980455238ac2bd

lix/libstore/build/derivation-goal.cc

@@ -756,7 +756,6 @@ retry:
 
     if (buildMode != bmCheck && allValid) {
         debug("skipping build of derivation '%s', someone beat us to it", worker.store.printStorePath(drvPath));
-        outputLocks.setDeletion(true);
         co_return done(BuildResult::AlreadyValid, std::move(validOutputs));
     }
 
@@ -1087,7 +1086,6 @@ try {
            lockers will see that the output paths are valid; they will
            not create new lock files with the same names as the old
            (unlinked) lock files. */
-        outputLocks.setDeletion(true);
         outputLocks.unlock();
 
         co_return done(BuildResult::Built, std::move(builtOutputs));

lix/libstore/build/local-derivation-goal.cc

@@ -2296,7 +2296,6 @@ try {
            floating CA derivations and hash-mismatching fixed-output
            derivations. */
         PathLocks dynamicOutputLock;
-        dynamicOutputLock.setDeletion(true);
         auto optFixedPath = output->path(worker.store, drv->name, outputName);
         if (!optFixedPath ||
             worker.store.printStorePath(*optFixedPath) != finalDestPath)

lix/libstore/local-store.cc

@@ -1272,8 +1272,6 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source,
 
             registerValidPath(info);
         }
-
-        outputLock.setDeletion(true);
     }
 }
 
@@ -1414,8 +1412,6 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name
             info.narSize = narHash.second;
             registerValidPath(info);
         }
-
-        outputLock.setDeletion(true);
     }
 
     return dstPath;
@@ -1466,8 +1462,6 @@ StorePath LocalStore::addTextToStore(
             };
             registerValidPath(info);
         }
-
-        outputLock.setDeletion(true);
     }
 
     return dstPath;

lix/libstore/pathlocks.cc

@@ -26,18 +26,6 @@ AutoCloseFD openLockFile(const Path & path, bool create)
 }
 
 
-void deleteLockFile(const Path & path, int fd)
-{
-    /* Get rid of the lock file.  Have to be careful not to introduce
-       races.  Write a (meaningless) token to the file to indicate to
-       other processes waiting on this lock that the lock is stale
-       (deleted). */
-    unlink(path.c_str());
-    writeFull(fd, "d");
-    /* Note that the result of unlink() is ignored; removing the lock
-       file is an optimisation, not a necessity. */
-}
-
 static int convertLockType(LockType lockType)
 {
     if (lockType == ltRead) return LOCK_SH;
@@ -144,13 +132,11 @@ void unlockFile(int fd)
 
 
 PathLocks::PathLocks()
-    : deletePaths(false)
 {
 }
 
 
 PathLocks::PathLocks(const PathSet & paths, const std::string & waitMsg)
-    : deletePaths(false)
 {
     lockPaths(paths, waitMsg);
 }
@@ -200,7 +186,7 @@ bool PathLocks::lockPathsImpl(const PathSet & paths,
             struct stat st;
             if (fstat(fd.get(), &st) == -1)
                 throw SysError("statting lock file '%1%'", lockPath);
-            if (st.st_size != 0)
+            if (st.st_nlink == 0)
                 /* This lock file has been unlinked, so we're holding
                    a lock on a deleted file.  This means that other
                    processes may create and acquire a lock on
@@ -231,12 +217,17 @@ PathLocks::~PathLocks()
 void PathLocks::unlock()
 {
     for (auto & i : fds) {
-        if (deletePaths) deleteLockFile(i.second, i.first);
+        // delete the file. if another file descriptor is used to acquire a lock on
+        // this file it will figure out that the file is stale once it calls stat()
+        // and inspects the link count. if unlink fails we merely leave around some
+        // stale lock file paths that can be reused or cleaned up by other threads.
+        unlink(i.second.c_str());
+        // clobber file contents for compatibility wither other nix implementations
+        writeFull(i.first, "d");
 
-        if (close(i.first) == -1)
-            printError(
-                "error (ignored): cannot close lock file on '%1%'",
-                i.second);
+        if (close(i.first) == -1) {
+            printError("error (ignored): cannot close lock file on '%1%'", i.second);
+        }
 
         debug("lock released on '%1%'", i.second);
     }
@@ -245,12 +236,6 @@ void PathLocks::unlock()
 }
 
 
-void PathLocks::setDeletion(bool deletePaths)
-{
-    this->deletePaths = deletePaths;
-}
-
-
 FdLock::FdLock(AutoCloseFD & fd, LockType lockType, DontWait)
 {
     if (tryLockFile(fd.get(), lockType)) {

lix/libstore/pathlocks.hh

@@ -17,11 +17,6 @@ namespace nix {
  */
 AutoCloseFD openLockFile(const Path & path, bool create);
 
-/**
- * Delete an open lock file.
- */
-void deleteLockFile(const Path & path, int fd);
-
 enum LockType { ltRead, ltWrite };
 
 void lockFile(int fd, LockType lockType);
@@ -41,7 +36,6 @@ class PathLocks
 private:
     typedef std::pair<int, Path> FDPair;
     std::list<FDPair> fds;
-    bool deletePaths;
 
     bool lockPathsImpl(const PathSet & _paths, const std::string & waitMsg, bool wait);
 
@@ -58,7 +52,6 @@ public:
     }
     ~PathLocks();
     void unlock();
-    void setDeletion(bool deletePaths);
 };
 
 class FdLock

lix/libstore/profiles.cc

@@ -294,7 +294,6 @@ void switchGeneration(
 void lockProfile(PathLocks & lock, const Path & profile)
 {
     lock.lockPaths({profile}, fmt("waiting for lock on profile '%1%'", profile));
-    lock.setDeletion(true);
 }