Remove timeout from Goal

This removes the last additional entry point into Goal's subclasses,
such that the CFG in the subclasses is now trivial to see.

Author: L-as

Diff for: src/libstore/build/derivation-goal.cc

@@ -96,22 +96,6 @@ std::string DerivationGoal::key()
 }
 
 
-void DerivationGoal::killChild()
-{
-#ifndef _WIN32 // TODO enable build hook on Windows
-    hook.reset();
-#endif
-}
-
-
-void DerivationGoal::timedOut(Error && ex)
-{
-    killChild();
-    // We're not inside a coroutine, hence we can't use co_return here.
-    // Thus we ignore the return value.
-    [[maybe_unused]] Done _ = done(BuildResult::TimedOut, {}, std::move(ex));
-}
-
 void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
 {
     auto newWanted = wantedOutputs.union_(outputs);
@@ -778,11 +762,12 @@ Goal::Co DerivationGoal::tryToBuild()
                     currentHookLine,
                     logTail
                 );
+                bool timedOut = false;
 #ifndef _WIN32 // TODO enable build hook on Windows
                 co_await childStarted(
                     { hook->fromHook.readSide.get()
                     , hook->builderOut.readSide.get()
-                    }, false, false, std::move(handler));
+                    }, false, false, std::move(handler), timedOut);
                 trace("calling childStarted hook end");
 #endif
 
@@ -793,7 +778,12 @@ Goal::Co DerivationGoal::tryToBuild()
                     act->result(resBuildLogLine, currentLogLine);
                 }
 
-                if (failed) {
+                if (timedOut) {
+#ifndef _WIN32 // TODO enable build hook on Windows
+                    hook.reset();
+#endif
+                    co_return done(BuildResult::TimedOut, {}, std::move(ex));
+                } else if (failed) {
                     co_return done(
                         BuildResult::LogLimitExceeded,
                         {},

Diff for: src/libstore/build/derivation-goal.hh

@@ -209,8 +209,6 @@ struct DerivationGoal : public Goal
         BuildMode buildMode = bmNormal);
     virtual ~DerivationGoal();
 
-    void timedOut(Error && ex) override;
-
     std::string key() override;
 
     /**
@@ -267,11 +265,6 @@ struct DerivationGoal : public Goal
      */
     SingleDrvOutputs assertPathValidity();
 
-    /**
-     * Forcibly kill the child process, if any.
-     */
-    virtual void killChild();
-
     Co repairClosure();
 
     void started();

Diff for: src/libstore/build/drv-output-substitution-goal.cc

@@ -58,13 +58,15 @@ Goal::Co DrvOutputSubstitutionGoal::init()
                 }
             } });
 
+        bool timedOut = false;
         co_await childStarted({
     #ifndef _WIN32
             outPipe->readSide.get()
     #else
             &*outPipe
     #endif
-        }, true, false, [](Descriptor, std::string_view) {return false;});
+        }, true, false, [](Descriptor, std::string_view) {return false;}, timedOut);
+        assert(!timedOut);
 
         /*
          * The realisation corresponding to the given output id.

Diff for: src/libstore/build/drv-output-substitution-goal.hh

@@ -36,8 +36,6 @@ public:
     Co init() override;
     Co realisationFetched(Goals waitees, std::shared_ptr<const Realisation> outputInfo, nix::ref<nix::Store> sub);
 
-    void timedOut(Error && ex) override { unreachable(); };
-
     std::string key() override;
 
     JobCategory jobCategory() const override {

Diff for: src/libstore/build/goal.cc

@@ -244,18 +244,26 @@ void Goal::handleChildOutput(Descriptor fd, std::string_view data) {
     }
 }
 
+void Goal::timedOut(Error && ex) {
+    didTimeOut = true;
+    worker.wakeUp(shared_from_this());
+}
+
 Co Goal::childStarted(
     const std::set<MuxablePipePollState::CommChannel> & channels,
     bool inBuildSlot,
     bool respectTimeouts,
-    std::function<bool(Descriptor fd, std::string_view data)> handler
+    std::function<bool(Descriptor fd, std::string_view data)> handler,
+    bool& timedOut
 ) {
     worker.childStarted(shared_from_this(), channels, inBuildSlot, respectTimeouts);
+    didTimeOut = false;
     assert(handler);
     childHandler = std::move(handler);
     co_await Suspend{};
     trace("done listening to children");
     childHandler = {};
+    timedOut = didTimeOut;
     worker.childTerminated(this);
     co_return Return{};
 }

Diff for: src/libstore/build/goal.hh

@@ -411,7 +411,7 @@ public:
      * get rid of any running child processes that are being monitored
      * by the worker (important!), etc.
      */
-    virtual void timedOut(Error && ex) = 0;
+    void timedOut(Error && ex);
 
     virtual std::string key() = 0;
 
@@ -424,6 +424,8 @@ public:
 private:
     std::function<bool(Descriptor, std::string_view)> childHandler;
 
+    bool didTimeOut;
+
 protected:
     Co await(Goals waitees);
 
@@ -435,7 +437,8 @@ protected:
         const std::set<MuxablePipePollState::CommChannel> & channels,
         bool inBuildSlot,
         bool respectTimeouts,
-        std::function<bool(Descriptor, std::string_view)> handler
+        std::function<bool(Descriptor, std::string_view)> handler,
+        bool& timedOut
     );
 };
 

Diff for: src/libstore/build/substitution-goal.cc

@@ -219,13 +219,15 @@ Goal::Co PathSubstitutionGoal::tryToRun(StorePath subPath, nix::ref<Store> sub,
         }
     });
 
+    bool timedOut = false;
     co_await childStarted({
 #ifndef _WIN32
         outPipe.readSide.get()
 #else
         &outPipe
 #endif
-    }, true, false, [](Descriptor, std::string_view) {return false;});
+    }, true, false, [](Descriptor, std::string_view) {return false;}, timedOut);
+    assert(!timedOut);
 
     trace("substitute finished");
 

Diff for: src/libstore/build/substitution-goal.hh

@@ -50,8 +50,6 @@ public:
     PathSubstitutionGoal(const StorePath & storePath, Worker & worker, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt);
     ~PathSubstitutionGoal();
 
-    void timedOut(Error && ex) override { unreachable(); };
-
     /**
      * We prepend "a$" to the key name to ensure substitution goals
      * happen before derivation goals.

Diff for: src/libstore/unix/build/local-derivation-goal.cc

@@ -319,7 +319,7 @@ struct LocalDerivationGoal : DerivationGoal, RestrictionContext
      *
      * Called by destructor, can't be overridden
      */
-    void killChild() override final;
+    void killChild();
 
     /**
      * Kill any processes running under the build user UID or in the
@@ -442,8 +442,6 @@ void LocalDerivationGoal::killChild()
 
         pid.wait();
     }
-
-    DerivationGoal::killChild();
 }
 
 
@@ -814,7 +812,6 @@ static std::function<bool(Descriptor, std::string_view)> build_handler(
     std::string & currentLogLine,
     size_t & currentLogLinePos,
     std::list<std::string> & logTail
-
 ) {
     return [&](Descriptor fd, std::string_view data) {
         // local & `ssh://`-builds are dealt with here.
@@ -1511,7 +1508,8 @@ Goal::Co LocalDerivationGoal::startBuilder(std::list<std::string> & logTail)
         logTail
     );
 
-    co_await childStarted({builderOut.get()}, true, true, std::move(handler));
+    bool timedOut = false;
+    co_await childStarted({builderOut.get()}, true, true, std::move(handler), timedOut);
 
     trace("calling childStarted build end");
 
@@ -1522,7 +1520,10 @@ Goal::Co LocalDerivationGoal::startBuilder(std::list<std::string> & logTail)
         act->result(resBuildLogLine, currentLogLine);
     }
 
-    if (failed) {
+    if (timedOut) {
+        killChild();
+        co_return done(BuildResult::TimedOut, {}, std::move(ex));
+    } if (failed) {
         killChild();
         co_return done(
             BuildResult::LogLimitExceeded,