libstore: return finishedness from Goal methods

this is the first step towards removing all result-related mutation of
Goal state from goal implementations themselves, and into Worker state
instead. once that is done we can treat all non-const Goal fields like
private state of the goal itself, and make threading of goals possible

Change-Id: I69ff7d02a6fd91a65887c6640bfc4f5fb785b45c

src/libstore/build/derivation-goal.cc

@@ -130,16 +130,16 @@ void DerivationGoal::killChild()
 }
 
 
-void DerivationGoal::timedOut(Error && ex)
+Goal::Finished DerivationGoal::timedOut(Error && ex)
 {
     killChild();
-    done(BuildResult::TimedOut, {}, std::move(ex));
+    return done(BuildResult::TimedOut, {}, std::move(ex));
 }
 
 
-void DerivationGoal::work()
+Goal::WorkResult DerivationGoal::work()
 {
-    (this->*state)();
+    return (this->*state)();
 }
 
 void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
@@ -163,7 +163,7 @@ void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
 }
 
 
-void DerivationGoal::getDerivation()
+Goal::WorkResult DerivationGoal::getDerivation()
 {
     trace("init");
 
@@ -171,23 +171,22 @@ void DerivationGoal::getDerivation()
        exists.  If it doesn't, it may be created through a
        substitute. */
     if (buildMode == bmNormal && worker.evalStore.isValidPath(drvPath)) {
-        loadDerivation();
+        return loadDerivation();
-        return;
     }
 
     addWaitee(worker.makePathSubstitutionGoal(drvPath));
 
     state = &DerivationGoal::loadDerivation;
+    return StillAlive{};
 }
 
 
-void DerivationGoal::loadDerivation()
+Goal::WorkResult DerivationGoal::loadDerivation()
 {
     trace("loading derivation");
 
     if (nrFailed != 0) {
-        done(BuildResult::MiscFailure, {}, Error("cannot build missing derivation '%s'", worker.store.printStorePath(drvPath)));
+        return done(BuildResult::MiscFailure, {}, Error("cannot build missing derivation '%s'", worker.store.printStorePath(drvPath)));
-        return;
     }
 
     /* `drvPath' should already be a root, but let's be on the safe
@@ -209,11 +208,11 @@ void DerivationGoal::loadDerivation()
     }
     assert(drv);
 
-    haveDerivation();
+    return haveDerivation();
 }
 
 
-void DerivationGoal::haveDerivation()
+Goal::WorkResult DerivationGoal::haveDerivation()
 {
     trace("have derivation");
 
@@ -241,8 +240,7 @@ void DerivationGoal::haveDerivation()
             });
         }
 
-        gaveUpOnSubstitution();
+        return gaveUpOnSubstitution();
-        return;
     }
 
     for (auto & i : drv->outputsAndOptPaths(worker.store))
@@ -264,8 +262,7 @@ void DerivationGoal::haveDerivation()
 
     /* If they are all valid, then we're done. */
     if (allValid && buildMode == bmNormal) {
-        done(BuildResult::AlreadyValid, std::move(validOutputs));
+        return done(BuildResult::AlreadyValid, std::move(validOutputs));
-        return;
     }
 
     /* We are first going to try to create the invalid output paths
@@ -290,24 +287,24 @@ void DerivationGoal::haveDerivation()
             }
         }
 
-    if (waitees.empty()) /* to prevent hang (no wake-up event) */
+    if (waitees.empty()) { /* to prevent hang (no wake-up event) */
-        outputsSubstitutionTried();
+        return outputsSubstitutionTried();
-    else
+    } else {
         state = &DerivationGoal::outputsSubstitutionTried;
+        return StillAlive{};
+    }
 }
 
-
+Goal::WorkResult DerivationGoal::outputsSubstitutionTried()
-void DerivationGoal::outputsSubstitutionTried()
 {
     trace("all outputs substituted (maybe)");
 
     assert(drv->type().isPure());
 
     if (nrFailed > 0 && nrFailed > nrNoSubstituters + nrIncompleteClosure && !settings.tryFallback) {
-        done(BuildResult::TransientFailure, {},
+        return done(BuildResult::TransientFailure, {},
             Error("some substitutes for the outputs of derivation '%s' failed (usually happens due to networking issues); try '--fallback' to build derivation from source ",
                 worker.store.printStorePath(drvPath)));
-        return;
     }
 
     /*  If the substitutes form an incomplete closure, then we should
@@ -341,32 +338,29 @@ void DerivationGoal::outputsSubstitutionTried()
 
     if (needRestart == NeedRestartForMoreOutputs::OutputsAddedDoNeed) {
         needRestart = NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed;
-        haveDerivation();
+        return haveDerivation();
-        return;
     }
 
     auto [allValid, validOutputs] = checkPathValidity();
 
     if (buildMode == bmNormal && allValid) {
-        done(BuildResult::Substituted, std::move(validOutputs));
+        return done(BuildResult::Substituted, std::move(validOutputs));
-        return;
     }
     if (buildMode == bmRepair && allValid) {
-        repairClosure();
+        return repairClosure();
-        return;
     }
     if (buildMode == bmCheck && !allValid)
         throw Error("some outputs of '%s' are not valid, so checking is not possible",
             worker.store.printStorePath(drvPath));
 
     /* Nothing to wait for; tail call */
-    gaveUpOnSubstitution();
+    return gaveUpOnSubstitution();
 }
 
 
 /* At least one of the output paths could not be
    produced using a substitute.  So we have to build instead. */
-void DerivationGoal::gaveUpOnSubstitution()
+Goal::WorkResult DerivationGoal::gaveUpOnSubstitution()
 {
     /* At this point we are building all outputs, so if more are wanted there
        is no need to restart. */
@@ -427,14 +421,16 @@ void DerivationGoal::gaveUpOnSubstitution()
         addWaitee(worker.makePathSubstitutionGoal(i));
     }
 
-    if (waitees.empty()) /* to prevent hang (no wake-up event) */
+    if (waitees.empty()) {/* to prevent hang (no wake-up event) */
-        inputsRealised();
+        return inputsRealised();
-    else
+    } else {
         state = &DerivationGoal::inputsRealised;
+        return StillAlive{};
+    }
 }
 
 
-void DerivationGoal::repairClosure()
+Goal::WorkResult DerivationGoal::repairClosure()
 {
     assert(drv->type().isPure());
 
@@ -488,41 +484,39 @@ void DerivationGoal::repairClosure()
     }
 
     if (waitees.empty()) {
-        done(BuildResult::AlreadyValid, assertPathValidity());
+        return done(BuildResult::AlreadyValid, assertPathValidity());
-        return;
     }
 
     state = &DerivationGoal::closureRepaired;
+    return StillAlive{};
 }
 
 
-void DerivationGoal::closureRepaired()
+Goal::WorkResult DerivationGoal::closureRepaired()
 {
     trace("closure repaired");
     if (nrFailed > 0)
         throw Error("some paths in the output closure of derivation '%s' could not be repaired",
             worker.store.printStorePath(drvPath));
-    done(BuildResult::AlreadyValid, assertPathValidity());
+    return done(BuildResult::AlreadyValid, assertPathValidity());
 }
 
 
-void DerivationGoal::inputsRealised()
+Goal::WorkResult DerivationGoal::inputsRealised()
 {
     trace("all inputs realised");
 
     if (nrFailed != 0) {
         if (!useDerivation)
             throw Error("some dependencies of '%s' are missing", worker.store.printStorePath(drvPath));
-        done(BuildResult::DependencyFailed, {}, Error(
+        return done(BuildResult::DependencyFailed, {}, Error(
                 "%s dependencies of derivation '%s' failed to build",
                 nrFailed, worker.store.printStorePath(drvPath)));
-        return;
     }
 
     if (retrySubstitution == RetrySubstitution::YesNeed) {
         retrySubstitution = RetrySubstitution::AlreadyRetried;
-        haveDerivation();
+        return haveDerivation();
-        return;
     }
 
     /* Gather information necessary for computing the closure and/or
@@ -589,7 +583,7 @@ void DerivationGoal::inputsRealised()
             addWaitee(resolvedDrvGoal);
 
             state = &DerivationGoal::resolvedFinished;
-            return;
+            return StillAlive{};
         }
 
         std::function<void(const StorePath &, const DerivedPathMap<StringSet>::ChildNode &)> accumInputPaths;
@@ -655,9 +649,10 @@ void DerivationGoal::inputsRealised()
        build hook. */
     state = &DerivationGoal::tryToBuild;
     worker.wakeUp(shared_from_this());
+    return StillAlive{};
 }
 
-void DerivationGoal::started()
+Goal::WorkResult DerivationGoal::started()
 {
     auto msg = fmt(
         buildMode == bmRepair ? "repairing outputs of '%s'" :
@@ -668,9 +663,10 @@ void DerivationGoal::started()
     act = std::make_unique<Activity>(*logger, lvlInfo, actBuild, msg,
         Logger::Fields{worker.store.printStorePath(drvPath), hook ? machineName : "", 1, 1});
     mcRunningBuilds = std::make_unique<MaintainCount<uint64_t>>(worker.runningBuilds);
+    return StillAlive{};
 }
 
-void DerivationGoal::tryToBuild()
+Goal::WorkResult DerivationGoal::tryToBuild()
 {
     trace("trying to build");
 
@@ -706,7 +702,7 @@ void DerivationGoal::tryToBuild()
             actLock = std::make_unique<Activity>(*logger, lvlWarn, actBuildWaiting,
                 fmt("waiting for lock on %s", Magenta(showPaths(lockFiles))));
         worker.waitForAWhile(shared_from_this());
-        return;
+        return StillAlive{};
     }
 
     actLock.reset();
@@ -723,8 +719,7 @@ void DerivationGoal::tryToBuild()
     if (buildMode != bmCheck && allValid) {
         debug("skipping build of derivation '%s', someone beat us to it", worker.store.printStorePath(drvPath));
         outputLocks.setDeletion(true);
-        done(BuildResult::AlreadyValid, std::move(validOutputs));
+        return done(BuildResult::AlreadyValid, std::move(validOutputs));
-        return;
     }
 
     /* If any of the outputs already exist but are not valid, delete
@@ -751,8 +746,7 @@ void DerivationGoal::tryToBuild()
                 actLock.reset();
                 buildResult.startTime = time(0); // inexact
                 state = &DerivationGoal::buildDone;
-                started();
+                return started();
-                return;
             case rpPostpone:
                 /* Not now; wait until at least one child finishes or
                    the wake-up timeout expires. */
@@ -761,7 +755,7 @@ void DerivationGoal::tryToBuild()
                         fmt("waiting for a machine to build '%s'", Magenta(worker.store.printStorePath(drvPath))));
                 worker.waitForAWhile(shared_from_this());
                 outputLocks.unlock();
-                return;
+                return StillAlive{};
             case rpDecline:
                 /* We should do it ourselves. */
                 break;
@@ -772,9 +766,10 @@ void DerivationGoal::tryToBuild()
 
     state = &DerivationGoal::tryLocalBuild;
     worker.wakeUp(shared_from_this());
+    return StillAlive{};
 }
 
-void DerivationGoal::tryLocalBuild() {
+Goal::WorkResult DerivationGoal::tryLocalBuild() {
     throw Error(
         "unable to build with a primary store that isn't a local store; "
         "either pass a different '--store' or enable remote builds."
@@ -932,7 +927,7 @@ void runPostBuildHook(
     proc.getStdout()->drainInto(sink);
 }
 
-void DerivationGoal::buildDone()
+Goal::WorkResult DerivationGoal::buildDone()
 {
     trace("build done");
 
@@ -1027,8 +1022,7 @@ void DerivationGoal::buildDone()
         outputLocks.setDeletion(true);
         outputLocks.unlock();
 
-        done(BuildResult::Built, std::move(builtOutputs));
+        return done(BuildResult::Built, std::move(builtOutputs));
-
     } catch (BuildError & e) {
         outputLocks.unlock();
 
@@ -1049,12 +1043,11 @@ void DerivationGoal::buildDone()
                 BuildResult::PermanentFailure;
         }
 
-        done(st, {}, std::move(e));
+        return done(st, {}, std::move(e));
-        return;
     }
 }
 
-void DerivationGoal::resolvedFinished()
+Goal::WorkResult DerivationGoal::resolvedFinished()
 {
     trace("resolved derivation finished");
 
@@ -1122,7 +1115,7 @@ void DerivationGoal::resolvedFinished()
     if (status == BuildResult::AlreadyValid)
         status = BuildResult::ResolvesToAlreadyValid;
 
-    done(status, std::move(builtOutputs));
+    return done(status, std::move(builtOutputs));
 }
 
 HookReply DerivationGoal::tryBuildHook()
@@ -1293,7 +1286,7 @@ bool DerivationGoal::isReadDesc(int fd)
     return fd == hook->builderOut.readSide.get();
 }
 
-void DerivationGoal::handleChildOutput(int fd, std::string_view data)
+Goal::WorkResult DerivationGoal::handleChildOutput(int fd, std::string_view data)
 {
     // local & `ssh://`-builds are dealt with here.
     auto isWrittenToLog = isReadDesc(fd);
@@ -1302,11 +1295,10 @@ void DerivationGoal::handleChildOutput(int fd, std::string_view data)
         logSize += data.size();
         if (settings.maxLogSize && logSize > settings.maxLogSize) {
             killChild();
-            done(
+            return done(
                 BuildResult::LogLimitExceeded, {},
                 Error("%s killed after writing more than %d bytes of log output",
                     getName(), settings.maxLogSize));
-            return;
         }
 
         for (auto c : data)
@@ -1357,6 +1349,8 @@ void DerivationGoal::handleChildOutput(int fd, std::string_view data)
             } else
                 currentHookLine += c;
     }
+
+    return StillAlive{};
 }
 
 
@@ -1505,7 +1499,7 @@ SingleDrvOutputs DerivationGoal::assertPathValidity()
 }
 
 
-void DerivationGoal::done(
+Goal::Finished DerivationGoal::done(
     BuildResult::Status status,
     SingleDrvOutputs builtOutputs,
     std::optional<Error> ex)
@@ -1540,7 +1534,7 @@ void DerivationGoal::done(
         fs << worker.store.printStorePath(drvPath) << "\t" << buildResult.toString() << std::endl;
     }
 
-    amDone(buildResult.success() ? ecSuccess : ecFailed, std::move(ex));
+    return amDone(buildResult.success() ? ecSuccess : ecFailed, std::move(ex));
 }
 
 

src/libstore/build/derivation-goal.hh

@@ -188,7 +188,7 @@ struct DerivationGoal : public Goal
      */
     std::optional<DerivationType> derivationType;
 
-    typedef void (DerivationGoal::*GoalState)();
+    typedef WorkResult (DerivationGoal::*GoalState)();
     GoalState state;
 
     BuildMode buildMode;
@@ -217,11 +217,11 @@ struct DerivationGoal : public Goal
         BuildMode buildMode = bmNormal);
     virtual ~DerivationGoal() noexcept(false);
 
-    void timedOut(Error && ex) override;
+    Finished timedOut(Error && ex) override;
 
     std::string key() override;
 
-    void work() override;
+    WorkResult work() override;
 
     /**
      * Add wanted outputs to an already existing derivation goal.
@@ -231,18 +231,18 @@ struct DerivationGoal : public Goal
     /**
      * The states.
      */
-    void getDerivation();
+    WorkResult getDerivation();
-    void loadDerivation();
+    WorkResult loadDerivation();
-    void haveDerivation();
+    WorkResult haveDerivation();
-    void outputsSubstitutionTried();
+    WorkResult outputsSubstitutionTried();
-    void gaveUpOnSubstitution();
+    WorkResult gaveUpOnSubstitution();
-    void closureRepaired();
+    WorkResult closureRepaired();
-    void inputsRealised();
+    WorkResult inputsRealised();
-    void tryToBuild();
+    WorkResult tryToBuild();
-    virtual void tryLocalBuild();
+    virtual WorkResult tryLocalBuild();
-    void buildDone();
+    WorkResult buildDone();
 
-    void resolvedFinished();
+    WorkResult resolvedFinished();
 
     /**
      * Is the build hook willing to perform the build?
@@ -292,7 +292,7 @@ struct DerivationGoal : public Goal
     /**
      * Callback used by the worker to write to the log.
      */
-    void handleChildOutput(int fd, std::string_view data) override;
+    WorkResult handleChildOutput(int fd, std::string_view data) override;
     void handleEOF(int fd) override;
     void flushLine();
 
@@ -323,11 +323,11 @@ struct DerivationGoal : public Goal
      */
     virtual void killChild();
 
-    void repairClosure();
+    WorkResult repairClosure();
 
-    void started();
+    WorkResult started();
 
-    void done(
+    Finished done(
         BuildResult::Status status,
         SingleDrvOutputs builtOutputs = {},
         std::optional<Error> ex = {});

src/libstore/build/drv-output-substitution-goal.cc

@@ -20,21 +20,20 @@ DrvOutputSubstitutionGoal::DrvOutputSubstitutionGoal(
 }
 
 
-void DrvOutputSubstitutionGoal::init()
+Goal::WorkResult DrvOutputSubstitutionGoal::init()
 {
     trace("init");
 
     /* If the derivation already exists, we’re done */
     if (worker.store.queryRealisation(id)) {
-        amDone(ecSuccess);
+        return amDone(ecSuccess);
-        return;
     }
 
     subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>();
-    tryNext();
+    return tryNext();
 }
 
-void DrvOutputSubstitutionGoal::tryNext()
+Goal::WorkResult DrvOutputSubstitutionGoal::tryNext()
 {
     trace("trying next substituter");
 
@@ -43,7 +42,7 @@ void DrvOutputSubstitutionGoal::tryNext()
        prevents infinite waiting. */
     if (worker.runningSubstitutions >= std::max(1U, settings.maxSubstitutionJobs.get())) {
         worker.waitForBuildSlot(shared_from_this());
-        return;
+        return StillAlive{};
     }
 
     maintainRunningSubstitutions =
@@ -54,16 +53,14 @@ void DrvOutputSubstitutionGoal::tryNext()
            with it. */
         debug("derivation output '%s' is required, but there is no substituter that can provide it", id.to_string());
 
-        /* Hack: don't indicate failure if there were no substituters.
-           In that case the calling derivation should just do a
-           build. */
-        amDone(substituterFailed ? ecFailed : ecNoSubstituters);
-
         if (substituterFailed) {
             worker.failedSubstitutions++;
         }
 
-        return;
+        /* Hack: don't indicate failure if there were no substituters.
+           In that case the calling derivation should just do a
+           build. */
+        return amDone(substituterFailed ? ecFailed : ecNoSubstituters);
     }
 
     sub = subs.front();
@@ -85,9 +82,10 @@ void DrvOutputSubstitutionGoal::tryNext()
     worker.childStarted(shared_from_this(), {downloadState->outPipe.readSide.get()}, true, false);
 
     state = &DrvOutputSubstitutionGoal::realisationFetched;
+    return StillAlive{};
 }
 
-void DrvOutputSubstitutionGoal::realisationFetched()
+Goal::WorkResult DrvOutputSubstitutionGoal::realisationFetched()
 {
     worker.childTerminated(this);
     maintainRunningSubstitutions.reset();
@@ -116,8 +114,7 @@ void DrvOutputSubstitutionGoal::realisationFetched()
                     worker.store.printStorePath(localOutputInfo->outPath),
                     worker.store.printStorePath(depPath)
                 );
-                tryNext();
+                return tryNext();
-                return;
             }
             addWaitee(worker.makeDrvOutputSubstitutionGoal(depId));
         }
@@ -125,29 +122,32 @@ void DrvOutputSubstitutionGoal::realisationFetched()
 
     addWaitee(worker.makePathSubstitutionGoal(outputInfo->outPath));
 
-    if (waitees.empty()) outPathValid();
+    if (waitees.empty()) {
-    else state = &DrvOutputSubstitutionGoal::outPathValid;
+        return outPathValid();
+    } else {
+        state = &DrvOutputSubstitutionGoal::outPathValid;
+        return StillAlive{};
+    }
 }
 
-void DrvOutputSubstitutionGoal::outPathValid()
+Goal::WorkResult DrvOutputSubstitutionGoal::outPathValid()
 {
     assert(outputInfo);
     trace("output path substituted");
 
     if (nrFailed > 0) {
         debug("The output path of the derivation output '%s' could not be substituted", id.to_string());
-        amDone(nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed);
+        return amDone(nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed);
-        return;
     }
 
     worker.store.registerDrvOutput(*outputInfo);
-    finished();
+    return finished();
 }
 
-void DrvOutputSubstitutionGoal::finished()
+Goal::WorkResult DrvOutputSubstitutionGoal::finished()
 {
     trace("finished");
-    amDone(ecSuccess);
+    return amDone(ecSuccess);
 }
 
 std::string DrvOutputSubstitutionGoal::key()
@@ -157,9 +157,9 @@ std::string DrvOutputSubstitutionGoal::key()
     return "a$" + std::string(id.to_string());
 }
 
-void DrvOutputSubstitutionGoal::work()
+Goal::WorkResult DrvOutputSubstitutionGoal::work()
 {
-    (this->*state)();
+    return (this->*state)();
 }
 
 

src/libstore/build/drv-output-substitution-goal.hh

@@ -58,20 +58,20 @@ class DrvOutputSubstitutionGoal : public Goal {
 public:
     DrvOutputSubstitutionGoal(const DrvOutput& id, Worker & worker, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt);
 
-    typedef void (DrvOutputSubstitutionGoal::*GoalState)();
+    typedef WorkResult (DrvOutputSubstitutionGoal::*GoalState)();
     GoalState state;
 
-    void init();
+    WorkResult init();
-    void tryNext();
+    WorkResult tryNext();
-    void realisationFetched();
+    WorkResult realisationFetched();
-    void outPathValid();
+    WorkResult outPathValid();
-    void finished();
+    WorkResult finished();
 
-    void timedOut(Error && ex) override { abort(); };
+    Finished timedOut(Error && ex) override { abort(); };
 
     std::string key() override;
 
-    void work() override;
+    WorkResult work() override;
 
     JobCategory jobCategory() const override {
         return JobCategory::Substitution;

src/libstore/build/goal.cc

@@ -45,7 +45,7 @@ void Goal::waiteeDone(GoalPtr waitee, ExitCode result)
 }
 
 
-void Goal::amDone(ExitCode result, std::optional<Error> ex)
+Goal::Finished Goal::amDone(ExitCode result, std::optional<Error> ex)
 {
     trace("done");
     assert(!exitCode.has_value());
@@ -66,6 +66,7 @@ void Goal::amDone(ExitCode result, std::optional<Error> ex)
     worker.removeGoal(shared_from_this());
 
     cleanup();
+    return Finished{};
 }
 
 

src/libstore/build/goal.hh

@@ -105,6 +105,15 @@ struct Goal : public std::enable_shared_from_this<Goal>
 
 public:
 
+    struct StillAlive {};
+    struct Finished {};
+
+    struct WorkResult : std::variant<StillAlive, Finished>
+    {
+        WorkResult() = delete;
+        using variant::variant;
+    };
+
     /**
      * Exception containing an error message, if any.
      */
@@ -119,13 +128,13 @@ public:
         trace("goal destroyed");
     }
 
-    virtual void work() = 0;
+    virtual WorkResult work() = 0;
 
     void addWaitee(GoalPtr waitee);
 
     virtual void waiteeDone(GoalPtr waitee, ExitCode result);
 
-    virtual void handleChildOutput(int fd, std::string_view data)
+    virtual WorkResult handleChildOutput(int fd, std::string_view data)
     {
         abort();
     }
@@ -146,11 +155,11 @@ public:
      * get rid of any running child processes that are being monitored
      * by the worker (important!), etc.
      */
-    virtual void timedOut(Error && ex) = 0;
+    virtual Finished timedOut(Error && ex) = 0;
 
     virtual std::string key() = 0;
 
-    void amDone(ExitCode result, std::optional<Error> ex = {});
+    Finished amDone(ExitCode result, std::optional<Error> ex = {});
 
     virtual void cleanup() { }
 

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

@@ -161,7 +161,7 @@ void LocalDerivationGoal::killSandbox(bool getStats)
 }
 
 
-void LocalDerivationGoal::tryLocalBuild()
+Goal::WorkResult LocalDerivationGoal::tryLocalBuild()
 {
 #if __APPLE__
     additionalSandboxProfile = parsedDrv->getStringAttr("__sandboxProfile").value_or("");
@@ -172,7 +172,7 @@ void LocalDerivationGoal::tryLocalBuild()
         state = &DerivationGoal::tryToBuild;
         worker.waitForBuildSlot(shared_from_this());
         outputLocks.unlock();
-        return;
+        return StillAlive{};
     }
 
     assert(derivationType);
@@ -215,7 +215,7 @@ void LocalDerivationGoal::tryLocalBuild()
                 actLock = std::make_unique<Activity>(*logger, lvlWarn, actBuildWaiting,
                     fmt("waiting for a free build user ID for '%s'", Magenta(worker.store.printStorePath(drvPath))));
             worker.waitForAWhile(shared_from_this());
-            return;
+            return StillAlive{};
         }
     }
 
@@ -250,15 +250,14 @@ void LocalDerivationGoal::tryLocalBuild()
         outputLocks.unlock();
         buildUser.reset();
         worker.permanentFailure = true;
-        done(BuildResult::InputRejected, {}, std::move(e));
+        return done(BuildResult::InputRejected, {}, std::move(e));
-        return;
     }
 
     /* This state will be reached when we get EOF on the child's
        log pipe. */
     state = &DerivationGoal::buildDone;
 
-    started();
+    return started();
 }
 
 

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

@@ -211,7 +211,7 @@ struct LocalDerivationGoal : public DerivationGoal
     /**
      * The additional states.
      */
-    void tryLocalBuild() override;
+    WorkResult tryLocalBuild() override;
 
     /**
      * Start building a derivation.

src/libstore/build/substitution-goal.cc

@@ -25,7 +25,7 @@ PathSubstitutionGoal::~PathSubstitutionGoal()
 }
 
 
-void PathSubstitutionGoal::done(
+Goal::Finished PathSubstitutionGoal::done(
     ExitCode result,
     BuildResult::Status status,
     std::optional<std::string> errorMsg)
@@ -35,17 +35,17 @@ void PathSubstitutionGoal::done(
         debug(*errorMsg);
         buildResult.errorMsg = *errorMsg;
     }
-    amDone(result);
+    return amDone(result);
 }
 
 
-void PathSubstitutionGoal::work()
+Goal::WorkResult PathSubstitutionGoal::work()
 {
-    (this->*state)();
+    return (this->*state)();
 }
 
 
-void PathSubstitutionGoal::init()
+Goal::WorkResult PathSubstitutionGoal::init()
 {
     trace("init");
 
@@ -53,8 +53,7 @@ void PathSubstitutionGoal::init()
 
     /* If the path already exists we're done. */
     if (!repair && worker.store.isValidPath(storePath)) {
-        done(ecSuccess, BuildResult::AlreadyValid);
+        return done(ecSuccess, BuildResult::AlreadyValid);
-        return;
     }
 
     if (settings.readOnlyMode)
@@ -62,11 +61,11 @@ void PathSubstitutionGoal::init()
 
     subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>();
 
-    tryNext();
+    return tryNext();
 }
 
 
-void PathSubstitutionGoal::tryNext()
+Goal::WorkResult PathSubstitutionGoal::tryNext()
 {
     trace("trying next substituter");
 
@@ -75,20 +74,17 @@ void PathSubstitutionGoal::tryNext()
     if (subs.size() == 0) {
         /* None left.  Terminate this goal and let someone else deal
            with it. */
-
-        /* Hack: don't indicate failure if there were no substituters.
-           In that case the calling derivation should just do a
-           build. */
-        done(
-            substituterFailed ? ecFailed : ecNoSubstituters,
-            BuildResult::NoSubstituters,
-            fmt("path '%s' is required, but there is no substituter that can build it", worker.store.printStorePath(storePath)));
-
         if (substituterFailed) {
             worker.failedSubstitutions++;
         }
 
-        return;
+        /* Hack: don't indicate failure if there were no substituters.
+           In that case the calling derivation should just do a
+           build. */
+        return done(
+            substituterFailed ? ecFailed : ecNoSubstituters,
+            BuildResult::NoSubstituters,
+            fmt("path '%s' is required, but there is no substituter that can build it", worker.store.printStorePath(storePath)));
     }
 
     sub = subs.front();
@@ -101,27 +97,23 @@ void PathSubstitutionGoal::tryNext()
         if (sub->storeDir == worker.store.storeDir)
             assert(subPath == storePath);
     } else if (sub->storeDir != worker.store.storeDir) {
-        tryNext();
+        return tryNext();
-        return;
     }
 
     try {
         // FIXME: make async
         info = sub->queryPathInfo(subPath ? *subPath : storePath);
     } catch (InvalidPath &) {
-        tryNext();
+        return tryNext();
-        return;
     } catch (SubstituterDisabled &) {
         if (settings.tryFallback) {
-            tryNext();
+            return tryNext();
-            return;
         }
         throw;
     } catch (Error & e) {
         if (settings.tryFallback) {
             logError(e.info());
-            tryNext();
+            return tryNext();
-            return;
         }
         throw;
     }
@@ -134,8 +126,7 @@ void PathSubstitutionGoal::tryNext()
         } else {
             printError("asked '%s' for '%s' but got '%s'",
                 sub->getUri(), worker.store.printStorePath(storePath), sub->printStorePath(info->path));
-            tryNext();
+            return tryNext();
-            return;
         }
     }
 
@@ -156,8 +147,7 @@ void PathSubstitutionGoal::tryNext()
     {
         warn("ignoring substitute for '%s' from '%s', as it's not signed by any of the keys in 'trusted-public-keys'",
             worker.store.printStorePath(storePath), sub->getUri());
-        tryNext();
+        return tryNext();
-        return;
     }
 
     /* To maintain the closure invariant, we first have to realise the
@@ -166,23 +156,24 @@ void PathSubstitutionGoal::tryNext()
         if (i != storePath) /* ignore self-references */
             addWaitee(worker.makePathSubstitutionGoal(i));
 
-    if (waitees.empty()) /* to prevent hang (no wake-up event) */
+    if (waitees.empty()) {/* to prevent hang (no wake-up event) */
-        referencesValid();
+        return referencesValid();
-    else
+    } else {
         state = &PathSubstitutionGoal::referencesValid;
+        return StillAlive{};
+    }
 }
 
 
-void PathSubstitutionGoal::referencesValid()
+Goal::WorkResult PathSubstitutionGoal::referencesValid()
 {
     trace("all references realised");
 
     if (nrFailed > 0) {
-        done(
+        return done(
             nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed,
             BuildResult::DependencyFailed,
             fmt("some references of path '%s' could not be realised", worker.store.printStorePath(storePath)));
-        return;
     }
 
     for (auto & i : info->references)
@@ -191,10 +182,11 @@ void PathSubstitutionGoal::referencesValid()
 
     state = &PathSubstitutionGoal::tryToRun;
     worker.wakeUp(shared_from_this());
+    return StillAlive{};
 }
 
 
-void PathSubstitutionGoal::tryToRun()
+Goal::WorkResult PathSubstitutionGoal::tryToRun()
 {
     trace("trying to run");
 
@@ -203,7 +195,7 @@ void PathSubstitutionGoal::tryToRun()
        prevents infinite waiting. */
     if (worker.getNrSubstitutions() >= std::max(1U, (unsigned int) settings.maxSubstitutionJobs)) {
         worker.waitForBuildSlot(shared_from_this());
-        return;
+        return StillAlive{};
     }
 
     maintainRunningSubstitutions = std::make_unique<MaintainCount<uint64_t>>(worker.runningSubstitutions);
@@ -236,10 +228,11 @@ void PathSubstitutionGoal::tryToRun()
     worker.childStarted(shared_from_this(), {outPipe.readSide.get()}, true, false);
 
     state = &PathSubstitutionGoal::finished;
+    return StillAlive{};
 }
 
 
-void PathSubstitutionGoal::finished()
+Goal::WorkResult PathSubstitutionGoal::finished()
 {
     trace("substitute finished");
 
@@ -264,7 +257,7 @@ void PathSubstitutionGoal::finished()
         /* Try the next substitute. */
         state = &PathSubstitutionGoal::tryNext;
         worker.wakeUp(shared_from_this());
-        return;
+        return StillAlive{};
     }
 
     worker.markContentsGood(storePath);
@@ -285,12 +278,13 @@ void PathSubstitutionGoal::finished()
     worker.doneNarSize += maintainExpectedNar->delta;
     maintainExpectedNar.reset();
 
-    done(ecSuccess, BuildResult::Substituted);
+    return done(ecSuccess, BuildResult::Substituted);
 }
 
 
-void PathSubstitutionGoal::handleChildOutput(int fd, std::string_view data)
+Goal::WorkResult PathSubstitutionGoal::handleChildOutput(int fd, std::string_view data)
 {
+    return StillAlive{};
 }
 
 

src/libstore/build/substitution-goal.hh

@@ -66,7 +66,7 @@ struct PathSubstitutionGoal : public Goal
     std::unique_ptr<MaintainCount<uint64_t>> maintainExpectedSubstitutions,
         maintainRunningSubstitutions, maintainExpectedNar, maintainExpectedDownload;
 
-    typedef void (PathSubstitutionGoal::*GoalState)();
+    typedef WorkResult (PathSubstitutionGoal::*GoalState)();
     GoalState state;
 
     /**
@@ -74,7 +74,7 @@ struct PathSubstitutionGoal : public Goal
      */
     std::optional<ContentAddress> ca;
 
-    void done(
+    Finished done(
         ExitCode result,
         BuildResult::Status status,
         std::optional<std::string> errorMsg = {});
@@ -83,7 +83,7 @@ public:
     PathSubstitutionGoal(const StorePath & storePath, Worker & worker, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt);
     ~PathSubstitutionGoal();
 
-    void timedOut(Error && ex) override { abort(); };
+    Finished timedOut(Error && ex) override { abort(); };
 
     /**
      * We prepend "a$" to the key name to ensure substitution goals
@@ -94,22 +94,22 @@ public:
         return "a$" + std::string(storePath.name()) + "$" + worker.store.printStorePath(storePath);
     }
 
-    void work() override;
+    WorkResult work() override;
 
     /**
      * The states.
      */
-    void init();
+    WorkResult init();
-    void tryNext();
+    WorkResult tryNext();
-    void gotInfo();
+    WorkResult gotInfo();
-    void referencesValid();
+    WorkResult referencesValid();
-    void tryToRun();
+    WorkResult tryToRun();
-    void finished();
+    WorkResult finished();
 
     /**
      * Callback used by the worker to write to the log.
      */
-    void handleChildOutput(int fd, std::string_view data) override;
+    WorkResult handleChildOutput(int fd, std::string_view data) override;
 
     /* Called by destructor, can't be overridden */
     void cleanup() override final;