Merge pull request #12630 from L-as/me/clean-up-drv-goal

Clean up derivation goals a bit

src/libstore/build/derivation-goal.cc

@@ -649,7 +649,7 @@ Goal::Co DerivationGoal::tryToBuild()
                 buildResult.startTime = time(0); // inexact
                 started();
                 co_await Suspend{};
-                co_return buildDone();
+                co_return hookDone();
             case rpPostpone:
                 /* Not now; wait until at least one child finishes or
                    the wake-up timeout expires. */
@@ -810,55 +810,6 @@ void replaceValidPath(const Path & storePath, const Path & tmpPath)
 }
 
 
-int DerivationGoal::getChildStatus()
-{
-#ifndef _WIN32 // TODO enable build hook on Windows
-    return hook->pid.kill();
-#else
-    return 0;
-#endif
-}
-
-
-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,
@@ -917,57 +868,13 @@ void runPostBuildHook(
     });
 }
 
-Goal::Co DerivationGoal::buildDone()
+
+void appendLogTailErrorMsg(
+    Worker & worker,
+    const StorePath & drvPath,
+    const std::list<std::string> & logTail,
+    std::string & msg)
 {
-    trace("build done");
-
-    Finally releaseBuildUser([&](){ this->cleanupHookFinally(); });
-
-    cleanupPreChildKill();
-
-    /* Since we got an EOF on the logger pipe, the builder is presumed
-       to have terminated.  In fact, the builder could also have
-       simply have closed its end of the pipe, so just to be sure,
-       kill it. */
-    int status = getChildStatus();
-
-    debug("builder process for '%s' finished", worker.store.printStorePath(drvPath));
-
-    buildResult.timesBuilt++;
-    buildResult.stopTime = time(0);
-
-    /* So the child is gone now. */
-    worker.childTerminated(this);
-
-    /* Close the read side of the logger pipe. */
-    closeReadPipes();
-
-    /* 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),
-            status,
-            ((double) buildResult.cpuUser->count()) / 1000000,
-            ((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));
-
     if (!logger->isVerbose() && !logTail.empty()) {
         msg += fmt(";\nlast %d log lines:\n", logTail.size());
         for (auto & line : logTail) {
@@ -985,16 +892,71 @@ Goal::Co DerivationGoal::buildDone()
             nixLogCommand,
             worker.store.printStorePath(drvPath));
     }
+}
 
-            if (diskFull)
-                msg += "\nnote: build failure may have been caused by lack of free disk space";
 
-            throw BuildError(msg);
+Goal::Co DerivationGoal::hookDone()
+{
+#ifndef _WIN32
+    assert(hook);
+#endif
+
+    trace("hook build done");
+
+    /* Since we got an EOF on the logger pipe, the builder is presumed
+       to have terminated.  In fact, the builder could also have
+       simply have closed its end of the pipe, so just to be sure,
+       kill it. */
+    int status =
+#ifndef _WIN32 // TODO enable build hook on Windows
+        hook->pid.kill();
+#else
+        0;
+#endif
+
+    debug("build hook for '%s' finished", worker.store.printStorePath(drvPath));
+
+    buildResult.timesBuilt++;
+    buildResult.stopTime = time(0);
+
+    /* So the child is gone now. */
+    worker.childTerminated(this);
+
+    /* Close the read side of the logger pipe. */
+#ifndef _WIN32 // TODO enable build hook on Windows
+    hook->builderOut.readSide.close();
+    hook->fromHook.readSide.close();
+#endif
+
+    /* Close the log file. */
+    closeLogFile();
+
+    /* Check the exit status. */
+    if (!statusOk(status)) {
+        auto msg = fmt("builder for '%s' %s",
+            Magenta(worker.store.printStorePath(drvPath)),
+            statusToString(status));
+
+        appendLogTailErrorMsg(worker, drvPath, logTail, msg);
+
+        outputLocks.unlock();
+
+        /* TODO (once again) support fine-grained error codes, see issue #12641. */
+
+        co_return done(BuildResult::MiscFailure, {}, BuildError(msg));
     }
 
     /* Compute the FS closure of the outputs and register them as
        being valid. */
-        auto builtOutputs = registerOutputs();
+    auto builtOutputs =
+        /* When using a build hook, the build hook can register the output
+           as valid (by doing `nix-store --import').  If so we don't have
+           to do anything here.
+
+           We can only early return when the outputs are known a priori. For
+           floating content-addressing derivations this isn't the case.
+         */
+        assertPathValidity();
 
     StorePathSet outputPaths;
     for (auto & [_, output] : builtOutputs)
@@ -1006,8 +968,6 @@ Goal::Co DerivationGoal::buildDone()
         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
@@ -1016,32 +976,6 @@ Goal::Co DerivationGoal::buildDone()
     outputLocks.unlock();
 
     co_return done(BuildResult::Built, std::move(builtOutputs));
-
-    } catch (BuildError & e) {
-        outputLocks.unlock();
-
-        BuildResult::Status st = BuildResult::MiscFailure;
-
-#ifndef _WIN32 // TODO abstract over proc exit status
-        if (hook && WIFEXITED(status) && WEXITSTATUS(status) == 101)
-            st = BuildResult::TimedOut;
-
-        else if (hook && (!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 :
-                BuildResult::PermanentFailure;
-        }
-
-        co_return done(st, {}, std::move(e));
-    }
 }
 
 Goal::Co DerivationGoal::resolvedFinished()
@@ -1093,7 +1027,7 @@ Goal::Co DerivationGoal::resolvedFinished()
                         : worker.store;
                     newRealisation.dependentRealisations = drvOutputReferences(worker.store, *drv, realisation.outPath, &drvStore);
                 }
-                signRealisation(newRealisation);
+                worker.store.signRealisation(newRealisation);
                 worker.store.registerDrvOutput(newRealisation);
             }
             outputPaths.insert(realisation.outPath);
@@ -1228,18 +1162,6 @@ HookReply DerivationGoal::tryBuildHook()
 }
 
 
-SingleDrvOutputs DerivationGoal::registerOutputs()
-{
-    /* When using a build hook, the build hook can register the output
-       as valid (by doing `nix-store --import').  If so we don't have
-       to do anything here.
-
-       We can only early return when the outputs are known a priori. For
-       floating content-addressing derivations this isn't the case.
-     */
-    return assertPathValidity();
-}
-
 Path DerivationGoal::openLogFile()
 {
     logSize = 0;

src/libstore/build/derivation-goal.hh

@@ -55,6 +55,20 @@ struct InitialOutput {
     std::optional<InitialOutputStatus> known;
 };
 
+/** Used internally */
+void runPostBuildHook(
+    Store & store,
+    Logger & logger,
+    const StorePath & drvPath,
+    const StorePathSet & outputPaths);
+
+/** Used internally */
+void appendLogTailErrorMsg(
+    Worker & worker,
+    const StorePath & drvPath,
+    const std::list<std::string> & logTail,
+    std::string & msg);
+
 /**
  * A goal for building some or all of the outputs of a derivation.
  */
@@ -232,7 +246,7 @@ struct DerivationGoal : public Goal
     Co gaveUpOnSubstitution();
     Co tryToBuild();
     virtual Co tryLocalBuild();
-    Co buildDone();
+    Co hookDone();
 
     Co resolvedFinished();
 
@@ -241,44 +255,16 @@ struct DerivationGoal : public Goal
      */
     HookReply tryBuildHook();
 
-    virtual int getChildStatus();
-
-    /**
-     * Check that the derivation outputs all exist and register them
-     * as valid.
-     */
-    virtual SingleDrvOutputs registerOutputs();
-
     /**
      * Open a log file and a pipe to it.
      */
     Path openLogFile();
 
-    /**
-     * Sign the newly built realisation if the store allows it
-     */
-    virtual void signRealisation(Realisation&) {}
-
     /**
      * Close the log file.
      */
     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/local-store.cc

@@ -1585,19 +1585,6 @@ void LocalStore::addSignatures(const StorePath & storePath, const StringSet & si
 }
 
 
-void LocalStore::signRealisation(Realisation & realisation)
-{
-    // FIXME: keep secret keys in memory.
-
-    auto secretKeyFiles = settings.secretKeyFiles;
-
-    for (auto & secretKeyFile : secretKeyFiles.get()) {
-        SecretKey secretKey(readFile(secretKeyFile));
-        LocalSigner signer(std::move(secretKey));
-        realisation.sign(signer);
-    }
-}
-
 void LocalStore::signPathInfo(ValidPathInfo & info)
 {
     // FIXME: keep secret keys in memory.

src/libstore/local-store.hh

@@ -401,7 +401,6 @@ private:
      * specified by the ‘secret-key-files’ option.
      */
     void signPathInfo(ValidPathInfo & info);
-    void signRealisation(Realisation &);
 
     void addBuildLog(const StorePath & drvPath, std::string_view log) override;
 

src/libstore/store-api.cc

@@ -1274,6 +1274,19 @@ Derivation Store::readDerivation(const StorePath & drvPath)
 Derivation Store::readInvalidDerivation(const StorePath & drvPath)
 { return readDerivationCommon(*this, drvPath, false); }
 
+void Store::signRealisation(Realisation & realisation)
+{
+    // FIXME: keep secret keys in memory.
+
+    auto secretKeyFiles = settings.secretKeyFiles;
+
+    for (auto & secretKeyFile : secretKeyFiles.get()) {
+        SecretKey secretKey(readFile(secretKeyFile));
+        LocalSigner signer(std::move(secretKey));
+        realisation.sign(signer);
+    }
+}
+
 }
 
 

src/libstore/store-api.hh

@@ -622,6 +622,8 @@ public:
     virtual void addSignatures(const StorePath & storePath, const StringSet & sigs)
     { unsupported("addSignatures"); }
 
+    void signRealisation(Realisation &);
+
     /* Utility functions. */
 
     /**

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());
@@ -263,8 +265,122 @@ Goal::Co LocalDerivationGoal::tryLocalBuild()
 
     started();
     co_await Suspend{};
-    // after EOF on child
+
-    co_return buildDone();
+    trace("build done");
+
+    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();
+    });
+
+    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
+       simply have closed its end of the pipe, so just to be sure,
+       kill it. */
+    int status = pid.kill();
+
+    debug("builder process for '%s' finished", worker.store.printStorePath(drvPath));
+
+    buildResult.timesBuilt++;
+    buildResult.stopTime = time(0);
+
+    /* So the child is gone now. */
+    worker.childTerminated(this);
+
+    /* Close the read side of the logger pipe. */
+    builderOut.close();
+
+    /* Close the log file. */
+    closeLogFile();
+
+    /* 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",
+            worker.store.printStorePath(drvPath),
+            status,
+            ((double) buildResult.cpuUser->count()) / 1000000,
+            ((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);
+        }
+
+        /* Compute the FS closure of the outputs and register them as
+           being valid. */
+        auto builtOutputs = registerOutputs();
+
+        StorePathSet outputPaths;
+        for (auto & [_, output] : builtOutputs)
+            outputPaths.insert(output.outPath);
+        runPostBuildHook(
+            worker.store,
+            *logger,
+            drvPath,
+            outputPaths
+        );
+
+        /* 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
+           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));
+
+    } catch (BuildError & e) {
+        outputLocks.unlock();
+
+        assert(derivationType);
+        BuildResult::Status st =
+            dynamic_cast<NotDeterministic*>(&e) ? BuildResult::NotDeterministic :
+            statusOk(status) ? BuildResult::OutputRejected :
+            !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure :
+            BuildResult::PermanentFailure;
+
+        co_return done(st, {}, std::move(e));
+    }
 }
 
 static void chmod_(const Path & path, mode_t mode)
@@ -296,50 +412,6 @@ static void movePath(const Path & src, const Path & dst)
 extern void replaceValidPath(const Path & storePath, const Path & tmpPath);
 
 
-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()
 {
     bool diskFull = false;
@@ -380,24 +452,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);
@@ -727,7 +781,7 @@ void LocalDerivationGoal::startBuilder()
            environment using bind-mounts.  We put it in the Nix store
            so that the build outputs can be moved efficiently from the
            chroot to their final location. */
-        chrootParentDir = worker.store.Store::toRealPath(drvPath) + ".chroot";
+        auto chrootParentDir = worker.store.Store::toRealPath(drvPath) + ".chroot";
         deletePath(chrootParentDir);
 
         /* Clean up the chroot directory automatically. */
@@ -972,9 +1026,6 @@ void LocalDerivationGoal::startBuilder()
            us.
         */
 
-        if (derivationType->isSandboxed())
-            privateNetwork = true;
-
         userNamespaceSync.create();
 
         usingUserNamespace = userNamespacesSupported();
@@ -1002,7 +1053,7 @@ void LocalDerivationGoal::startBuilder()
 
                 ProcessOptions options;
                 options.cloneFlags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD;
-                if (privateNetwork)
+                if (derivationType->isSandboxed())
                     options.cloneFlags |= CLONE_NEWNET;
                 if (usingUserNamespace)
                     options.cloneFlags |= CLONE_NEWUSER;
@@ -1819,7 +1870,7 @@ void LocalDerivationGoal::runChild()
 
             userNamespaceSync.readSide = -1;
 
-            if (privateNetwork) {
+            if (derivationType->isSandboxed()) {
 
                 /* Initialise the loopback interface. */
                 AutoCloseFD fd(socket(PF_INET, SOCK_DGRAM, IPPROTO_IP));
@@ -2279,15 +2330,7 @@ void LocalDerivationGoal::runChild()
 
 SingleDrvOutputs LocalDerivationGoal::registerOutputs()
 {
-    /* When using a build hook, the build hook can register the output
+    assert(!hook);
-       as valid (by doing `nix-store --import').  If so we don't have
-       to do anything here.
-
-       We can only early return when the outputs are known a priori. For
-       floating content-addressing derivations this isn't the case.
-     */
-    if (hook)
-        return DerivationGoal::registerOutputs();
 
     std::map<std::string, ValidPathInfo> infos;
 
@@ -2829,7 +2872,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
         if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)
             && !drv->type().isImpure())
         {
-            signRealisation(thisRealisation);
+            worker.store.signRealisation(thisRealisation);
             worker.store.registerDrvOutput(thisRealisation);
         }
         builtOutputs.emplace(outputName, thisRealisation);
@@ -2838,11 +2881,6 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
     return builtOutputs;
 }
 
-void LocalDerivationGoal::signRealisation(Realisation & realisation)
-{
-    getLocalStore().signRealisation(realisation);
-}
-
 
 void LocalDerivationGoal::checkOutputs(const std::map<std::string, ValidPathInfo> & outputs)
 {

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

@@ -71,13 +71,6 @@ struct LocalDerivationGoal : public DerivationGoal
      */
     bool useChroot = false;
 
-    /**
-     * The parent directory of `chrootRootDir`. It has permission 700
-     * and is owned by root to ensure other users cannot mess with
-     * `chrootRootDir`.
-     */
-    Path chrootParentDir;
-
     /**
      * The root of the chroot environment.
      */
@@ -88,11 +81,6 @@ struct LocalDerivationGoal : public DerivationGoal
      */
     std::shared_ptr<AutoDelete> autoDelChroot;
 
-    /**
-     * Whether to run the build in a private network namespace.
-     */
-    bool privateNetwork = false;
-
     /**
      * Stuff we need to pass to initChild().
      */
@@ -242,8 +230,6 @@ struct LocalDerivationGoal : public DerivationGoal
      */
     void chownToBuilder(const Path & path);
 
-    int getChildStatus() override;
-
     /**
      * Run the builder's process.
      */
@@ -253,9 +239,7 @@ struct LocalDerivationGoal : public DerivationGoal
      * Check that the derivation outputs all exist and register them
      * as valid.
      */
-    SingleDrvOutputs registerOutputs() override;
+    SingleDrvOutputs registerOutputs();
-
-    void signRealisation(Realisation &) override;
 
     /**
      * Check that an output meets the requirements specified by the
@@ -264,21 +248,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 +268,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)