libstore: make Worker::childStarted private

this can be a proper WorkResult now. childTerminated is unfortunately a
lot more stubborn and won't be made private for quite a while yet. once
we can get rid of the Worker poll loop that *should* be possible though

Change-Id: I2218df202da5cb84e852f6a37e4c20367495b617

9 files changed, 35 insertions, 31 deletions

diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc
index a1f628afc..f31c3acd5 100644
--- a/src/libstore/build/derivation-goal.cc
+++ b/src/libstore/build/derivation-goal.cc
@@ -646,7 +646,7 @@ Goal::WorkResult DerivationGoal::inputsRealised(bool inBuildSlot)
     return ContinueImmediately{};
 }
 
-Goal::WorkResult DerivationGoal::started()
+void DerivationGoal::started()
 {
     auto msg = fmt(
         buildMode == bmRepair ? "repairing outputs of '%s'" :
@@ -657,7 +657,6 @@ Goal::WorkResult 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{};
 }
 
 Goal::WorkResult DerivationGoal::tryToBuild(bool inBuildSlot)
@@ -735,13 +734,14 @@ Goal::WorkResult DerivationGoal::tryToBuild(bool inBuildSlot)
         auto hookReply = tryBuildHook(inBuildSlot);
         auto result = std::visit(
             overloaded{
-                [&](HookReply::Accept) -> std::optional<WorkResult> {
+                [&](HookReply::Accept & a) -> std::optional<WorkResult> {
                     /* Yes, it has started doing so.  Wait until we get
                        EOF from the hook. */
                     actLock.reset();
                     buildResult.startTime = time(0); // inexact
                     state = &DerivationGoal::buildDone;
-                    return started();
+                    started();
+                    return WaitForWorld{std::move(a.fds), false};
                 },
                 [&](HookReply::Postpone) -> std::optional<WorkResult> {
                     /* Not now; wait until at least one child finishes or
@@ -1222,9 +1222,8 @@ HookReply DerivationGoal::tryBuildHook(bool inBuildSlot)
     fds.insert(hook->fromHook.get());
     fds.insert(hook->builderOut.get());
     builderOutFD = &hook->builderOut;
-    worker.childStarted(shared_from_this(), fds, false);
 
-    return HookReply::Accept{};
+    return HookReply::Accept{std::move(fds)};
 }
 
 
diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh
index f7d86ed2e..c9638c2b6 100644
--- a/src/libstore/build/derivation-goal.hh
+++ b/src/libstore/build/derivation-goal.hh
@@ -15,7 +15,9 @@ using std::map;
 struct HookInstance;
 
 struct HookReplyBase {
-    struct [[nodiscard]] Accept {};
+    struct [[nodiscard]] Accept {
+        std::set<int> fds;
+    };
     struct [[nodiscard]] Decline {};
     struct [[nodiscard]] Postpone {};
 };
@@ -345,7 +347,7 @@ struct DerivationGoal : public Goal
 
     WorkResult repairClosure();
 
-    WorkResult started();
+    void started();
 
     Finished done(
         BuildResult::Status status,
diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc
index aea055624..2032478d1 100644
--- a/src/libstore/build/drv-output-substitution-goal.cc
+++ b/src/libstore/build/drv-output-substitution-goal.cc
@@ -75,10 +75,8 @@ Goal::WorkResult DrvOutputSubstitutionGoal::tryNext(bool inBuildSlot)
             return sub->queryRealisation(id);
         });
 
-    worker.childStarted(shared_from_this(), {downloadState->outPipe.readSide.get()}, true);
-
     state = &DrvOutputSubstitutionGoal::realisationFetched;
-    return StillAlive{};
+    return WaitForWorld{{downloadState->outPipe.readSide.get()}, true};
 }
 
 Goal::WorkResult DrvOutputSubstitutionGoal::realisationFetched(bool inBuildSlot)
diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh
index 114abda41..2ac96ace1 100644
--- a/src/libstore/build/goal.hh
+++ b/src/libstore/build/goal.hh
@@ -112,6 +112,10 @@ public:
     struct [[nodiscard]] WaitForGoals {
         Goals goals;
     };
+    struct [[nodiscard]] WaitForWorld {
+        std::set<int> fds;
+        bool inBuildSlot;
+    };
     struct [[nodiscard]] Finished {
         ExitCode result;
         std::unique_ptr<Error> ex;
@@ -127,6 +131,7 @@ public:
                                           WaitForAWhile,
                                           ContinueImmediately,
                                           WaitForGoals,
+                                          WaitForWorld,
                                           Finished>
     {
         WorkResult() = delete;
diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc
index 25b520c05..1e3c4109d 100644
--- a/src/libstore/build/local-derivation-goal.cc
+++ b/src/libstore/build/local-derivation-goal.cc
@@ -230,7 +230,14 @@ Goal::WorkResult LocalDerivationGoal::tryLocalBuild(bool inBuildSlot)
     try {
 
         /* Okay, we have to build. */
-        startBuilder();
+        auto fds = startBuilder();
+
+        /* This state will be reached when we get EOF on the child's
+           log pipe. */
+        state = &DerivationGoal::buildDone;
+
+        started();
+        return WaitForWorld{std::move(fds), true};
 
     } catch (BuildError & e) {
         outputLocks.unlock();
@@ -239,12 +246,6 @@ Goal::WorkResult LocalDerivationGoal::tryLocalBuild(bool inBuildSlot)
         report.permanentFailure = true;
         return report;
     }
-
-    /* This state will be reached when we get EOF on the child's
-       log pipe. */
-    state = &DerivationGoal::buildDone;
-
-    return started();
 }
 
 
@@ -374,7 +375,7 @@ void LocalDerivationGoal::cleanupPostOutputsRegisteredModeNonCheck()
     cleanupPostOutputsRegisteredModeCheck();
 }
 
-void LocalDerivationGoal::startBuilder()
+std::set<int> LocalDerivationGoal::startBuilder()
 {
     if ((buildUser && buildUser->getUIDCount() != 1)
         #if __linux__
@@ -753,7 +754,7 @@ void LocalDerivationGoal::startBuilder()
         msgs.push_back(std::move(msg));
     }
 
-    worker.childStarted(shared_from_this(), {builderOutPTY.get()}, true);
+    return {builderOutPTY.get()};
 }
 
 
diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh
index 9b2391256..f17685af8 100644
--- a/src/libstore/build/local-derivation-goal.hh
+++ b/src/libstore/build/local-derivation-goal.hh
@@ -216,7 +216,7 @@ struct LocalDerivationGoal : public DerivationGoal
     /**
      * Start building a derivation.
      */
-    void startBuilder();
+    std::set<int> startBuilder();
 
     /**
      * Fill in the environment for the builder.
diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc
index 281ac216c..d013d1c78 100644
--- a/src/libstore/build/substitution-goal.cc
+++ b/src/libstore/build/substitution-goal.cc
@@ -221,10 +221,8 @@ Goal::WorkResult PathSubstitutionGoal::tryToRun(bool inBuildSlot)
         }
     });
 
-    worker.childStarted(shared_from_this(), {outPipe.readSide.get()}, true);
-
     state = &PathSubstitutionGoal::finished;
-    return StillAlive{};
+    return WaitForWorld{{outPipe.readSide.get()}, true};
 }
 
 
diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc
index 325de3073..9740cdd4d 100644
--- a/src/libstore/build/worker.cc
+++ b/src/libstore/build/worker.cc
@@ -198,6 +198,7 @@ void Worker::handleWorkResult(GoalPtr goal, Goal::WorkResult how)
                     dep->waiters.insert(goal);
                 }
             },
+            [&](Goal::WaitForWorld & w) { childStarted(goal, w.fds, w.inBuildSlot); },
             [&](Goal::Finished & f) { goalFinished(goal, f); },
         },
         how
diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh
index 406bf205d..7abb966f9 100644
--- a/src/libstore/build/worker.hh
+++ b/src/libstore/build/worker.hh
@@ -157,6 +157,13 @@ private:
      */
     void removeGoal(GoalPtr goal);
 
+    /**
+     * Registers a running child process.  `inBuildSlot` means that
+     * the process counts towards the jobs limit.
+     */
+    void childStarted(GoalPtr goal, const std::set<int> & fds,
+        bool inBuildSlot);
+
 public:
 
     const Activity act;
@@ -229,13 +236,6 @@ public:
     GoalPtr makeGoal(const DerivedPath & req, BuildMode buildMode = bmNormal);
 
     /**
-     * Registers a running child process.  `inBuildSlot` means that
-     * the process counts towards the jobs limit.
-     */
-    void childStarted(GoalPtr goal, const std::set<int> & fds,
-        bool inBuildSlot);
-
-    /**
      * Unregisters a running child process.
      */
     void childTerminated(Goal * goal);