Merge pull request #13860 from obsidiansystems/derivation-building-resources-code-cleanup

Derivation building resources code cleanup

src/libstore/build/derivation-building-goal.cc

@@ -54,24 +54,9 @@ DerivationBuildingGoal::~DerivationBuildingGoal()
 {
     /* Careful: we should never ever throw an exception from a
        destructor. */
-    try {
-        killChild();
-    } catch (...) {
-        ignoreExceptionInDestructor();
-    }
 #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows
-    if (builder) {
-        try {
-            builder->stopDaemon();
-        } catch (...) {
-            ignoreExceptionInDestructor();
-        }
-        try {
-            builder->deleteTmpDir(false);
-        } catch (...) {
-            ignoreExceptionInDestructor();
-        }
-    }
+    if (builder)
+        builder.reset();
 #endif
     try {
         closeLogFile();
@@ -95,22 +80,8 @@ void DerivationBuildingGoal::killChild()
     hook.reset();
 #endif
 #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows
-    if (builder && builder->pid != -1) {
+    if (builder && builder->killChild())
         worker.childTerminated(this);
-
-        // FIXME: move this into DerivationBuilder.
-
-        /* If we're using a build user, then there is a tricky race
-           condition: if we kill the build user before the child has
-           done its setuid() to the build user uid, then it won't be
-           killed, and we'll potentially lock up in pid.wait().  So
-           also send a conventional kill to the child. */
-        ::kill(-builder->pid, SIGKILL); /* ignore the result */
-
-        builder->killSandbox(true);
-
-        builder->pid.wait();
-    }
 #endif
 }
 
@@ -653,11 +624,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
                     goal.worker.childTerminated(&goal);
                 }
 
-                void markContentsGood(const StorePath & path) override
-                {
-                    goal.worker.markContentsGood(path);
-                }
-
                 Path openLogFile() override
                 {
                     return goal.openLogFile();
@@ -756,21 +722,22 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
 
     trace("build done");
 
-    auto res = builder->unprepareBuild();
-    // N.B. cannot use `std::visit` with co-routine return
-    if (auto * ste = std::get_if<0>(&res)) {
+    SingleDrvOutputs builtOutputs;
+    try {
+        builtOutputs = builder->unprepareBuild();
+    } catch (BuildError & e) {
         outputLocks.unlock();
 // Allow selecting a subset of enum values
 #  pragma GCC diagnostic push
 #  pragma GCC diagnostic ignored "-Wswitch-enum"
-        switch (ste->status) {
+        switch (e.status) {
         case BuildResult::HashMismatch:
             worker.hashMismatch = true;
             /* See header, the protocols don't know about `HashMismatch`
                yet, so change it to `OutputRejected`, which they expect
                for this case (hash mismatch is a type of output
                rejection). */
-            ste->status = BuildResult::OutputRejected;
+            e.status = BuildResult::OutputRejected;
             break;
         case BuildResult::NotDeterministic:
             worker.checkMismatch = true;
@@ -780,11 +747,15 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
             break;
         }
 #  pragma GCC diagnostic pop
-        co_return doneFailure(std::move(*ste));
-    } else if (auto * builtOutputs = std::get_if<1>(&res)) {
+        co_return doneFailure(std::move(e));
+    }
+    {
         StorePathSet outputPaths;
-        for (auto & [_, output] : *builtOutputs)
+        for (auto & [_, output] : builtOutputs) {
+            // for sake of `bmRepair`
+            worker.markContentsGood(output.outPath);
             outputPaths.insert(output.outPath);
+        }
         runPostBuildHook(worker.store, *logger, drvPath, outputPaths);
 
         /* It is now safe to delete the lock files, since all future
@@ -793,9 +764,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
            (unlinked) lock files. */
         outputLocks.setDeletion(true);
         outputLocks.unlock();
-        co_return doneSuccess(BuildResult::Built, std::move(*builtOutputs));
-    } else {
-        unreachable();
+        co_return doneSuccess(BuildResult::Built, std::move(builtOutputs));
     }
 #endif
 }

src/libstore/include/nix/store/build/derivation-builder.hh

@@ -107,8 +107,6 @@ struct DerivationBuilderCallbacks
      * @todo this should be reworked
      */
     virtual void childTerminated() = 0;
-
-    virtual void markContentsGood(const StorePath & path) = 0;
 };
 
 /**
@@ -124,11 +122,6 @@ struct DerivationBuilderCallbacks
  */
 struct DerivationBuilder : RestrictionContext
 {
-    /**
-     * The process ID of the builder.
-     */
-    Pid pid;
-
     DerivationBuilder() = default;
     virtual ~DerivationBuilder() = default;
 
@@ -161,25 +154,18 @@ struct DerivationBuilder : RestrictionContext
      * processing. A status code and exception are returned, providing
      * more information. The second case indicates success, and
      * realisations for each output of the derivation are returned.
+     *
+     * @throws BuildError
      */
-    virtual std::variant<BuildError, SingleDrvOutputs> unprepareBuild() = 0;
+    virtual SingleDrvOutputs unprepareBuild() = 0;
 
     /**
-     * Stop the in-process nix daemon thread.
-     * @see startDaemon
+     * Forcibly kill the child process, if any.
+     *
+     * @returns whether the child was still alive and needed to be
+     * killed.
      */
-    virtual void stopDaemon() = 0;
-
-    /**
-     * Delete the temporary directory, if we have one.
-     */
-    virtual void deleteTmpDir(bool force) = 0;
-
-    /**
-     * Kill any processes running under the build user UID or in the
-     * cgroup of the build.
-     */
-    virtual void killSandbox(bool getStats) = 0;
+    virtual bool killChild() = 0;
 };
 
 #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows

src/libstore/unix/build/chroot-derivation-builder.cc

@@ -22,13 +22,6 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl
 
     PathsInChroot pathsInChroot;
 
-    void deleteTmpDir(bool force) override
-    {
-        autoDelChroot.reset(); /* this runs the destructor */
-
-        DerivationBuilderImpl::deleteTmpDir(force);
-    }
-
     bool needsHashRewrite() override
     {
         return false;
@@ -166,13 +159,13 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl
         return !needsHashRewrite() ? chrootRootDir + p : store.toRealPath(p);
     }
 
-    void cleanupBuild() override
+    void cleanupBuild(bool force) override
     {
-        DerivationBuilderImpl::cleanupBuild();
+        DerivationBuilderImpl::cleanupBuild(force);
 
         /* Move paths out of the chroot for easier debugging of
            build failures. */
-        if (buildMode == bmNormal)
+        if (!force && buildMode == bmNormal)
             for (auto & [_, status] : initialOutputs) {
                 if (!status.known)
                     continue;
@@ -182,6 +175,8 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl
                 if (pathExists(chrootRootDir + p))
                     std::filesystem::rename((chrootRootDir + p), p);
             }
+
+        autoDelChroot.reset(); /* this runs the destructor */
     }
 
     std::pair<Path, Path> addDependencyPrep(const StorePath & path)

src/libstore/unix/build/derivation-builder.cc

@@ -71,6 +71,11 @@ class DerivationBuilderImpl : public DerivationBuilder, public DerivationBuilder
 {
 protected:
 
+    /**
+     * The process ID of the builder.
+     */
+    Pid pid;
+
     LocalStore & store;
 
     std::unique_ptr<DerivationBuilderCallbacks> miscMethods;
@@ -86,6 +91,27 @@ public:
     {
     }
 
+    ~DerivationBuilderImpl()
+    {
+        /* Careful: we should never ever throw an exception from a
+           destructor. */
+        try {
+            killChild();
+        } catch (...) {
+            ignoreExceptionInDestructor();
+        }
+        try {
+            stopDaemon();
+        } catch (...) {
+            ignoreExceptionInDestructor();
+        }
+        try {
+            cleanupBuild(false);
+        } catch (...) {
+            ignoreExceptionInDestructor();
+        }
+    }
+
 protected:
 
     /**
@@ -192,7 +218,7 @@ public:
 
     void startBuilder() override;
 
-    std::variant<BuildError, SingleDrvOutputs> unprepareBuild() override;
+    SingleDrvOutputs unprepareBuild() override;
 
 protected:
 
@@ -286,9 +312,11 @@ private:
      */
     void startDaemon();
 
-public:
-
-    void stopDaemon() override;
+    /**
+     * Stop the in-process nix daemon thread.
+     * @see startDaemon
+     */
+    void stopDaemon();
 
 protected:
 
@@ -343,15 +371,25 @@ private:
      */
     SingleDrvOutputs registerOutputs();
 
-public:
-
-    void deleteTmpDir(bool force) override;
-
-    void killSandbox(bool getStats) override;
-
 protected:
 
-    virtual void cleanupBuild();
+    /**
+     * Delete the temporary directory, if we have one.
+     *
+     * @param force We know the build suceeded, so don't attempt to
+     * preseve anything for debugging.
+     */
+    virtual void cleanupBuild(bool force);
+
+    /**
+     * Kill any processes running under the build user UID or in the
+     * cgroup of the build.
+     */
+    virtual void killSandbox(bool getStats);
+
+public:
+
+    bool killChild() override;
 
 private:
 
@@ -414,6 +452,24 @@ void DerivationBuilderImpl::killSandbox(bool getStats)
     }
 }
 
+bool DerivationBuilderImpl::killChild()
+{
+    bool ret = pid != -1;
+    if (ret) {
+        /* If we're using a build user, then there is a tricky race
+           condition: if we kill the build user before the child has
+           done its setuid() to the build user uid, then it won't be
+           killed, and we'll potentially lock up in pid.wait().  So
+           also send a conventional kill to the child. */
+        ::kill(-pid, SIGKILL); /* ignore the result */
+
+        killSandbox(true);
+
+        pid.wait();
+    }
+    return ret;
+}
+
 bool DerivationBuilderImpl::prepareBuild()
 {
     if (useBuildUsers()) {
@@ -427,7 +483,7 @@ bool DerivationBuilderImpl::prepareBuild()
     return true;
 }
 
-std::variant<BuildError, SingleDrvOutputs> DerivationBuilderImpl::unprepareBuild()
+SingleDrvOutputs DerivationBuilderImpl::unprepareBuild()
 {
     // FIXME: get rid of this, rely on RAII.
     Finally releaseBuildUser([&]() {
@@ -476,56 +532,42 @@ std::variant<BuildError, SingleDrvOutputs> DerivationBuilderImpl::unprepareBuild
             ((double) buildResult.cpuSystem->count()) / 1000000);
     }
 
-    bool diskFull = false;
+    /* Check the exit status. */
+    if (!statusOk(status)) {
 
-    try {
+        bool diskFull = decideWhetherDiskFull();
 
-        /* Check the exit status. */
-        if (!statusOk(status)) {
+        cleanupBuild(false);
 
-            diskFull |= decideWhetherDiskFull();
+        auto msg =
+            fmt("Cannot build '%s'.\n"
+                "Reason: " ANSI_RED "builder %s" ANSI_NORMAL ".",
+                Magenta(store.printStorePath(drvPath)),
+                statusToString(status));
 
-            cleanupBuild();
+        msg += showKnownOutputs(store, drv);
 
-            auto msg =
-                fmt("Cannot build '%s'.\n"
-                    "Reason: " ANSI_RED "builder %s" ANSI_NORMAL ".",
-                    Magenta(store.printStorePath(drvPath)),
-                    statusToString(status));
+        miscMethods->appendLogTailErrorMsg(msg);
 
-            msg += showKnownOutputs(store, drv);
+        if (diskFull)
+            msg += "\nnote: build failure may have been caused by lack of free disk space";
 
-            miscMethods->appendLogTailErrorMsg(msg);
-
-            if (diskFull)
-                msg += "\nnote: build failure may have been caused by lack of free disk space";
-
-            throw BuildError(
-                !derivationType.isSandboxed() || diskFull ? BuildResult::TransientFailure
-                                                          : BuildResult::PermanentFailure,
-                msg);
-        }
-
-        /* Compute the FS closure of the outputs and register them as
-           being valid. */
-        auto builtOutputs = registerOutputs();
-
-        /* Delete unused redirected outputs (when doing hash rewriting). */
-        for (auto & i : redirectedOutputs)
-            deletePath(store.Store::toRealPath(i.second));
-
-        deleteTmpDir(true);
-
-        return std::move(builtOutputs);
-
-    } catch (BuildError & e) {
-        return std::move(e);
+        throw BuildError(
+            !derivationType.isSandboxed() || diskFull ? BuildResult::TransientFailure : BuildResult::PermanentFailure,
+            msg);
     }
-}
 
-void DerivationBuilderImpl::cleanupBuild()
-{
-    deleteTmpDir(false);
+    /* Compute the FS closure of the outputs and register them as
+       being valid. */
+    auto builtOutputs = registerOutputs();
+
+    /* Delete unused redirected outputs (when doing hash rewriting). */
+    for (auto & i : redirectedOutputs)
+        deletePath(store.Store::toRealPath(i.second));
+
+    cleanupBuild(true);
+
+    return builtOutputs;
 }
 
 static void chmod_(const Path & path, mode_t mode)
@@ -1757,7 +1799,6 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
             }
 
             store.optimisePath(actualPath, NoRepair); // FIXME: combine with scanForReferences()
-            miscMethods->markContentsGood(newInfo.path);
 
             newInfo.deriver = drvPath;
             newInfo.ultimate = true;
@@ -1825,7 +1866,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
     return builtOutputs;
 }
 
-void DerivationBuilderImpl::deleteTmpDir(bool force)
+void DerivationBuilderImpl::cleanupBuild(bool force)
 {
     if (topTmpDir != "") {
         /* As an extra precaution, even in the event of `deletePath` failing to

src/libstore/unix/build/linux-derivation-builder.cc

@@ -659,7 +659,7 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu
             throw SysError("setuid failed");
     }
 
-    std::variant<BuildError, SingleDrvOutputs> unprepareBuild() override
+    SingleDrvOutputs unprepareBuild() override
     {
         sandboxMountNamespace = -1;
         sandboxUserNamespace = -1;