Use locks to prevent multiple actions accessing the same db files and chroot dirs

librepomgr/buildactions/buildactionprivate.h

@@ -138,7 +138,7 @@ public:
     template <typename... ChildArgs> void launch(ChildArgs &&...childArgs);
     void registerWebSession(std::shared_ptr<WebAPI::Session> &&webSession);
     void registerNewDataHandler(std::function<void(BufferType, std::size_t)> &&handler);
-    void assignLocks(AssociatedLocks &&locks = AssociatedLocks());
+    AssociatedLocks &locks();
     bool hasExited() const;
 
 private:
@@ -215,9 +215,9 @@ inline BuildProcessSession::BuildProcessSession(BuildAction *buildAction, boost:
 {
 }
 
-inline void BuildProcessSession::assignLocks(AssociatedLocks &&locks)
+inline AssociatedLocks &BuildProcessSession::locks()
 {
-    m_locks = std::move(locks);
+    return m_locks;
 }
 
 inline bool BuildProcessSession::hasExited() const
@@ -341,8 +341,10 @@ private:
 protected:
     std::string m_sourceRepoDirectory;
     std::string m_sourceDatabaseFile;
+    std::string m_sourceDatabaseLockName;
     std::string m_destinationRepoDirectory;
     std::string m_destinationDatabaseFile;
+    std::string m_destinationDatabaseLockName;
     std::string m_workingDirectory;
     std::vector<std::string> m_fileNames;
     PackageMovementResult m_result;

librepomgr/buildactions/conductbuild.cpp

@@ -901,19 +901,6 @@ InvocationResult ConductBuild::invokeMakechrootpkg(
         return InvocationResult::Error;
     }
 
-    // FIXME: lock the chroot directory to prevent other build tasks from using it (or does makechrootpkg already lock it?)
-
-    // copy config files into chroot directory
-    try {
-        std::filesystem::copy_file(makepkgchrootSession->isStagingEnabled() ? m_pacmanStagingConfigPath : m_pacmanConfigPath,
-            buildRoot + "/etc/pacman.conf", std::filesystem::copy_options::overwrite_existing);
-        std::filesystem::copy_file(m_makepkgConfigPath, buildRoot + "/etc/makepkg.conf", std::filesystem::copy_options::overwrite_existing);
-    } catch (const std::filesystem::filesystem_error &e) {
-        auto writeLock = lockToWrite(lock);
-        packageProgress.error = "Unable to configure chroot \"" % buildRoot % "\": " + e.what();
-        return InvocationResult::Error;
-    }
-
     // determine options/variables to pass
     std::vector<std::string> makechrootpkgFlags, makepkgFlags;
     // -> cleanup/upgrade
@@ -936,16 +923,35 @@ InvocationResult ConductBuild::invokeMakechrootpkg(
         makepkgFlags.emplace_back("TEST_FILE_PATH=/testfiles");
     }
 
-    // invoke makechrootpkg to build package
-    m_buildAction->log()(Phrases::InfoMessage, "Building ", packageName, '\n');
+    // prepare process session
     auto processSession = m_buildAction->makeBuildProcess(packageName + " build", packageProgress.buildDirectory + "/build.log",
         std::bind(&ConductBuild::handleMakechrootpkgErrorsAndAddPackageToRepo, this, makepkgchrootSession, std::ref(packageName),
             std::ref(packageProgress), std::placeholders::_1, std::placeholders::_2));
     processSession->registerNewDataHandler(BufferSearch("Updated version: ", "\e\n", "Starting build",
         std::bind(&ConductBuild::assignNewVersion, this, std::ref(packageName), std::ref(packageProgress), std::placeholders::_1)));
+    processSession->registerNewDataHandler(BufferSearch("Synchronizing chroot copy", "done", std::string_view(),
+        [processSession = processSession.get()](std::string &&) { processSession->locks().clear(); }));
+
+    // lock the chroot directory to prevent other build tasks from using it
+    m_buildAction->log()(Phrases::InfoMessage, "Building ", packageName, '\n');
+    auto chrootLock = m_setup.locks.acquireToWrite(buildRoot);
+
+    // copy config files into chroot directory
+    try {
+        std::filesystem::copy_file(makepkgchrootSession->isStagingEnabled() ? m_pacmanStagingConfigPath : m_pacmanConfigPath,
+            buildRoot + "/etc/pacman.conf", std::filesystem::copy_options::overwrite_existing);
+        std::filesystem::copy_file(m_makepkgConfigPath, buildRoot + "/etc/makepkg.conf", std::filesystem::copy_options::overwrite_existing);
+    } catch (const std::filesystem::filesystem_error &e) {
+        auto writeLock = lockToWrite(lock);
+        packageProgress.error = "Unable to configure chroot \"" % buildRoot % "\": " + e.what();
+        return InvocationResult::Error;
+    }
+
+    // invoke makechrootpkg to build package
     m_buildAction->log()(Phrases::InfoMessage, "Invoking makechrootpkg for ", packageName, " via ", m_makeChrootPkgPath.string(), '\n',
         ps(Phrases::SubMessage), "build dir: ", packageProgress.buildDirectory, '\n', ps(Phrases::SubMessage), "chroot dir: ", chrootDir, '\n',
         ps(Phrases::SubMessage), "chroot user: ", packageProgress.chrootUser, '\n');
+    processSession->locks().emplace_back(std::move(chrootLock));
     processSession->launch(boost::process::start_dir(packageProgress.buildDirectory), m_makeChrootPkgPath, makechrootpkgFlags, "-C",
         m_globalPackageCacheDir, "-r", chrootDir, "-l", packageProgress.chrootUser, packageProgress.makechrootpkgFlags, "--", makepkgFlags,
         packageProgress.makepkgFlags);
@@ -1111,6 +1117,8 @@ void ConductBuild::addPackageToRepo(
     auto processSession = m_buildAction->makeBuildProcess("repo-add for " + packageName, packageProgress.buildDirectory + "/repo-add.log",
         std::bind(&ConductBuild::handleRepoAddErrorsAndMakeNextPackage, this, makepkgchrootSession, std::ref(packageName), std::ref(packageProgress),
             std::placeholders::_1, std::placeholders::_2));
+    processSession->locks().emplace_back(m_setup.locks.acquireToWrite(
+        ServiceSetup::Locks::forDatabase(needsStaging ? m_buildPreparation.targetDb : m_buildPreparation.stagingDb, m_buildPreparation.targetArch)));
     processSession->launch(boost::process::start_dir(repoPath), m_repoAddPath, dbFilePath, binaryPackageNames);
     m_buildAction->log()(Phrases::InfoMessage, "Adding ", packageName, " to repo\n", ps(Phrases::SubMessage), "repo path: ", repoPath, '\n',
         ps(Phrases::SubMessage), "db path: ", dbFilePath, '\n', ps(Phrases::SubMessage), "package(s): ", joinStrings(binaryPackageNames), '\n');

librepomgr/buildactions/reloaddatabase.cpp

@@ -74,11 +74,13 @@ void ReloadDatabase::run()
                 m_buildAction->appendOutput(
                     Phrases::InfoMessage, "Loading database \"", dbName, '@', dbArch, "\" from local file \"", dbPath, "\"\n");
                 try {
+                    auto dbFileLock = m_setup.locks.acquireToRead({ ServiceSetup::Locks::forDatabase(dbName, dbArch) });
                     const auto lastModified = LibPkg::lastModified(dbPath);
                     auto dbFile = LibPkg::extractFiles(dbPath, &LibPkg::Database::isFileRelevant);
                     auto packages = LibPkg::Package::fromDatabaseFile(move(dbFile));
-                    auto lock = m_setup.config.lockToWrite();
-                    auto db = m_setup.config.findDatabase(dbName, dbArch);
+                    dbFileLock.unlock();
+                    const auto configLock = m_setup.config.lockToWrite();
+                    auto *const db = m_setup.config.findDatabase(dbName, dbArch);
                     if (!db) {
                         m_buildAction->appendOutput(
                             Phrases::ErrorMessage, "Loaded database file for \"", dbName, '@', dbArch, "\" but it no longer exists; discarding\n");

librepomgr/buildactions/repomanagement.cpp

@@ -55,10 +55,12 @@ bool PackageMovementAction::prepareRepoAction(RequiredDatabases requiredDatabase
     const auto *const destinationDb = *m_destinationDbs.begin();
     m_destinationRepoDirectory = destinationDb->localPkgDir;
     m_destinationDatabaseFile = fileName(destinationDb->path);
+    m_destinationDatabaseLockName = ServiceSetup::Locks::forDatabase(*destinationDb);
     if (requiredDatabases & RequiredDatabases::OneSource) {
         const auto *const sourceDb = *m_sourceDbs.begin();
         m_sourceRepoDirectory = sourceDb->localPkgDir;
         m_sourceDatabaseFile = fileName(sourceDb->path);
+        m_sourceDatabaseLockName = ServiceSetup::Locks::forDatabase(*sourceDb);
     }
     locatePackages();
     configReadLock = std::monostate();
@@ -154,6 +156,7 @@ void RemovePackages::run()
     // remove package from database file
     auto repoRemoveProcess = m_buildAction->makeBuildProcess("repo-remove", m_workingDirectory + "/repo-remove.log",
         std::bind(&RemovePackages::handleRepoRemoveResult, this, std::placeholders::_1, std::placeholders::_2));
+    repoRemoveProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_destinationDatabaseLockName));
     repoRemoveProcess->launch(
         boost::process::start_dir(m_destinationRepoDirectory), m_repoRemovePath, m_destinationDatabaseFile, m_result.processedPackages);
     m_buildAction->log()(Phrases::InfoMessage, "Invoking repo-remove within \"", m_destinationRepoDirectory, "\" for \"", m_destinationDatabaseFile,
@@ -270,11 +273,13 @@ void MovePackages::run()
     // add packages to database file of destination repo
     auto repoAddProcess = m_buildAction->makeBuildProcess("repo-add", m_workingDirectory + "/repo-add.log",
         std::bind(&MovePackages::handleRepoAddResult, this, processSession, std::placeholders::_1, std::placeholders::_2));
+    repoAddProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_destinationDatabaseLockName));
     repoAddProcess->launch(boost::process::start_dir(m_destinationRepoDirectory), m_repoAddPath, m_destinationDatabaseFile, m_fileNames);
 
     // remove package from database file of source repo
     auto repoRemoveProcess = m_buildAction->makeBuildProcess("repo-remove", m_workingDirectory + "/repo-remove.log",
         std::bind(&MovePackages::handleRepoRemoveResult, this, processSession, std::placeholders::_1, std::placeholders::_2));
+    repoRemoveProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_sourceDatabaseLockName));
     repoRemoveProcess->launch(boost::process::start_dir(m_sourceRepoDirectory), m_repoRemovePath, m_sourceDatabaseFile, m_result.processedPackages);
 
     m_buildAction->log()(ps(Phrases::InfoMessage), "Invoking repo-add within \"", m_destinationRepoDirectory, "\" for \"", m_destinationDatabaseFile,

librepomgr/serversetup.cpp

@@ -663,6 +663,16 @@ void ServiceSetup::Locks::clear()
     }
 }
 
+std::string ServiceSetup::Locks::forDatabase(std::string_view dbName, std::string_view dbArch)
+{
+    return dbName % '@' + dbArch;
+}
+
+std::string ServiceSetup::Locks::forDatabase(const LibPkg::Database &db)
+{
+    return forDatabase(db.name, db.arch);
+}
+
 ServiceStatus::ServiceStatus(const ServiceSetup &setup)
     : version(applicationInfo.version)
     , config(setup.config.computeStatus())

librepomgr/serversetup.h

@@ -125,6 +125,8 @@ struct LIBREPOMGR_EXPORT ServiceSetup : public LibPkg::Lockable {
         [[nodiscard]] std::unique_lock<std::shared_mutex> acquireToWrite(const std::string &lockName);
         [[nodiscard]] std::unique_lock<std::shared_mutex> acquireToWrite(std::shared_lock<std::shared_mutex> &readLock, const std::string &lockName);
         void clear();
+        static std::string forDatabase(std::string_view dbName, std::string_view dbArch);
+        static std::string forDatabase(const LibPkg::Database &db);
 
     private:
         std::mutex m_mutex;