Start simplifying {Local,}DerivationGoal cleanup code

Thanks to the previous commit, we can inline all these small callbacks.
In the build-hook case, they were empty, and now they disappear
entirely.

While `LocalDerivationGoal` can be used in the hook case (we use it
based on whether we have a local store, not based on whether we are
using the build hook, a decision which comes later), the previous
commit's inline moved the code into a spot where we know we are cleaning
up after local building, *not* after running the build hook. This allows
for much more simplification.

src/libstore/build/derivation-goal.cc

@@ -811,45 +811,6 @@ int DerivationGoal::getChildStatus()
 }
 
 
-void DerivationGoal::closeReadPipes()
-{
-#ifndef _WIN32 // TODO enable build hook on Windows
-    hook->builderOut.readSide.close();
-    hook->fromHook.readSide.close();
-#endif
-}
-
-
-void DerivationGoal::cleanupHookFinally()
-{
-}
-
-
-void DerivationGoal::cleanupPreChildKill()
-{
-}
-
-
-void DerivationGoal::cleanupPostChildKill()
-{
-}
-
-
-bool DerivationGoal::cleanupDecideWhetherDiskFull()
-{
-    return false;
-}
-
-
-void DerivationGoal::cleanupPostOutputsRegisteredModeCheck()
-{
-}
-
-
-void DerivationGoal::cleanupPostOutputsRegisteredModeNonCheck()
-{
-}
-
 void runPostBuildHook(
     Store & store,
     Logger & logger,
@@ -937,11 +898,11 @@ void appendLogTailErrorMsg(
 
 Goal::Co DerivationGoal::hookDone()
 {
-    trace("build done");
+#ifndef _WIN32
+    assert(hook);
+#endif
 
-    Finally releaseBuildUser([&](){ this->cleanupHookFinally(); });
+    trace("hook build done");
-
-    cleanupPreChildKill();
 
     /* Since we got an EOF on the logger pipe, the builder is presumed
        to have terminated.  In fact, the builder could also have
@@ -958,13 +919,14 @@ Goal::Co DerivationGoal::hookDone()
     worker.childTerminated(this);
 
     /* Close the read side of the logger pipe. */
-    closeReadPipes();
+#ifndef _WIN32 // TODO enable build hook on Windows
+    hook->builderOut.readSide.close();
+    hook->fromHook.readSide.close();
+#endif
 
     /* Close the log file. */
     closeLogFile();
 
-    cleanupPostChildKill();
-
     if (buildResult.cpuUser && buildResult.cpuSystem) {
         debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs",
             worker.store.printStorePath(drvPath),
@@ -973,24 +935,16 @@ Goal::Co DerivationGoal::hookDone()
             ((double) buildResult.cpuSystem->count()) / 1000000);
     }
 
-    bool diskFull = false;
-
     try {
 
         /* Check the exit status. */
         if (!statusOk(status)) {
-
-            diskFull |= cleanupDecideWhetherDiskFull();
-
             auto msg = fmt("builder for '%s' %s",
                 Magenta(worker.store.printStorePath(drvPath)),
                 statusToString(status));
 
             appendLogTailErrorMsg(worker, drvPath, logTail, msg);
 
-            if (diskFull)
-                msg += "\nnote: build failure may have been caused by lack of free disk space";
-
             throw BuildError(msg);
         }
 
@@ -1008,8 +962,6 @@ Goal::Co DerivationGoal::hookDone()
             outputPaths
         );
 
-        cleanupPostOutputsRegisteredModeNonCheck();
-
         /* It is now safe to delete the lock files, since all future
            lockers will see that the output paths are valid; they will
            not create new lock files with the same names as the old
@@ -1024,23 +976,20 @@ Goal::Co DerivationGoal::hookDone()
 
         BuildResult::Status st = BuildResult::MiscFailure;
 
-#ifndef _WIN32 // TODO abstract over proc exit status
+#ifndef _WIN32
-        if (hook && WIFEXITED(status) && WEXITSTATUS(status) == 101)
+        if (WIFEXITED(status) && WEXITSTATUS(status) == 101)
             st = BuildResult::TimedOut;
 
-        else if (hook && (!WIFEXITED(status) || WEXITSTATUS(status) != 100)) {
+        else if (WIFEXITED(status) && WEXITSTATUS(status) == 100)
-        }
-
-        else
-#endif
         {
             assert(derivationType);
             st =
                 dynamic_cast<NotDeterministic*>(&e) ? BuildResult::NotDeterministic :
                 statusOk(status) ? BuildResult::OutputRejected :
-                !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure :
+                !derivationType->isSandboxed() ? BuildResult::TransientFailure :
                 BuildResult::PermanentFailure;
         }
+#endif
 
         co_return done(st, {}, std::move(e));
     }

src/libstore/build/derivation-goal.hh

@@ -285,21 +285,6 @@ struct DerivationGoal : public Goal
      */
     void closeLogFile();
 
-    /**
-     * Close the read side of the logger pipe.
-     */
-    virtual void closeReadPipes();
-
-    /**
-     * Cleanup hooks for buildDone()
-     */
-    virtual void cleanupHookFinally();
-    virtual void cleanupPreChildKill();
-    virtual void cleanupPostChildKill();
-    virtual bool cleanupDecideWhetherDiskFull();
-    virtual void cleanupPostOutputsRegisteredModeCheck();
-    virtual void cleanupPostOutputsRegisteredModeNonCheck();
-
     virtual bool isReadDesc(Descriptor fd);
 
     /**

src/libstore/unix/build/local-derivation-goal.cc

@@ -184,6 +184,8 @@ void LocalDerivationGoal::killSandbox(bool getStats)
 
 Goal::Co LocalDerivationGoal::tryLocalBuild()
 {
+    assert(!hook);
+
     unsigned int curBuilds = worker.getNrLocalBuilds();
     if (curBuilds >= settings.maxBuildJobs) {
         worker.waitForBuildSlot(shared_from_this());
@@ -266,9 +268,15 @@ Goal::Co LocalDerivationGoal::tryLocalBuild()
 
     trace("build done");
 
-    Finally releaseBuildUser([&](){ this->cleanupHookFinally(); });
+    Finally releaseBuildUser([&](){
+        /* Release the build user at the end of this function. We don't do
+           it right away because we don't want another build grabbing this
+           uid and then messing around with our output. */
+        buildUser.reset();
+    });
 
-    cleanupPreChildKill();
+    sandboxMountNamespace = -1;
+    sandboxUserNamespace = -1;
 
     /* Since we got an EOF on the logger pipe, the builder is presumed
        to have terminated.  In fact, the builder could also have
@@ -285,12 +293,20 @@ Goal::Co LocalDerivationGoal::tryLocalBuild()
     worker.childTerminated(this);
 
     /* Close the read side of the logger pipe. */
-    closeReadPipes();
+    builderOut.close();
 
     /* Close the log file. */
     closeLogFile();
 
-    cleanupPostChildKill();
+    /* When running under a build user, make sure that all processes
+       running under that uid are gone.  This is to prevent a
+       malicious user from leaving behind a process that keeps files
+       open and modifies them after they have been chown'ed to
+       root. */
+    killSandbox(true);
+
+    /* Terminate the recursive Nix daemon. */
+    stopDaemon();
 
     if (buildResult.cpuUser && buildResult.cpuSystem) {
         debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs",
@@ -335,7 +351,14 @@ Goal::Co LocalDerivationGoal::tryLocalBuild()
             outputPaths
         );
 
-        cleanupPostOutputsRegisteredModeNonCheck();
+        /* Delete unused redirected outputs (when doing hash rewriting). */
+        for (auto & i : redirectedOutputs)
+            deletePath(worker.store.Store::toRealPath(i.second));
+
+        /* Delete the chroot (if we were using one). */
+        autoDelChroot.reset(); /* this runs the destructor */
+
+        deleteTmpDir(true);
 
         /* It is now safe to delete the lock files, since all future
            lockers will see that the output paths are valid; they will
@@ -407,44 +430,6 @@ int LocalDerivationGoal::getChildStatus()
     return hook ? DerivationGoal::getChildStatus() : pid.kill();
 }
 
-void LocalDerivationGoal::closeReadPipes()
-{
-    if (hook) {
-        DerivationGoal::closeReadPipes();
-    } else
-        builderOut.close();
-}
-
-
-void LocalDerivationGoal::cleanupHookFinally()
-{
-    /* Release the build user at the end of this function. We don't do
-       it right away because we don't want another build grabbing this
-       uid and then messing around with our output. */
-    buildUser.reset();
-}
-
-
-void LocalDerivationGoal::cleanupPreChildKill()
-{
-    sandboxMountNamespace = -1;
-    sandboxUserNamespace = -1;
-}
-
-
-void LocalDerivationGoal::cleanupPostChildKill()
-{
-    /* When running under a build user, make sure that all processes
-       running under that uid are gone.  This is to prevent a
-       malicious user from leaving behind a process that keeps files
-       open and modifies them after they have been chown'ed to
-       root. */
-    killSandbox(true);
-
-    /* Terminate the recursive Nix daemon. */
-    stopDaemon();
-}
-
 
 bool LocalDerivationGoal::cleanupDecideWhetherDiskFull()
 {
@@ -486,24 +471,6 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull()
 }
 
 
-void LocalDerivationGoal::cleanupPostOutputsRegisteredModeCheck()
-{
-    deleteTmpDir(true);
-}
-
-
-void LocalDerivationGoal::cleanupPostOutputsRegisteredModeNonCheck()
-{
-    /* Delete unused redirected outputs (when doing hash rewriting). */
-    for (auto & i : redirectedOutputs)
-        deletePath(worker.store.Store::toRealPath(i.second));
-
-    /* Delete the chroot (if we were using one). */
-    autoDelChroot.reset(); /* this runs the destructor */
-
-    cleanupPostOutputsRegisteredModeCheck();
-}
-
 #if __linux__
 static void doBind(const Path & source, const Path & target, bool optional = false) {
     debug("bind mounting '%1%' to '%2%'", source, target);

src/libstore/unix/build/local-derivation-goal.hh

@@ -264,21 +264,6 @@ struct LocalDerivationGoal : public DerivationGoal
      */
     void checkOutputs(const std::map<std::string, ValidPathInfo> & outputs);
 
-    /**
-     * Close the read side of the logger pipe.
-     */
-    void closeReadPipes() override;
-
-    /**
-     * Cleanup hooks for buildDone()
-     */
-    void cleanupHookFinally() override;
-    void cleanupPreChildKill() override;
-    void cleanupPostChildKill() override;
-    bool cleanupDecideWhetherDiskFull() override;
-    void cleanupPostOutputsRegisteredModeCheck() override;
-    void cleanupPostOutputsRegisteredModeNonCheck() override;
-
     bool isReadDesc(int fd) override;
 
     /**
@@ -299,6 +284,8 @@ struct LocalDerivationGoal : public DerivationGoal
      */
     void killSandbox(bool getStats);
 
+    bool cleanupDecideWhetherDiskFull();
+
     /**
      * Create alternative path calculated from but distinct from the
      * input, so we can avoid overwriting outputs (or other store paths)