Misc builder fixes

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

@@ -417,6 +417,10 @@ Goal::Co DerivationBuildingGoal::buildWithHook(
 #else
     std::unique_ptr<HookInstance> hook = std::move(worker.hook);
 
+    /* Set up callback so childTerminated is called if the hook is
+       destroyed (e.g., during failure cascades). */
+    hook->onKillChild = [this]() { worker.childTerminated(this, JobCategory::Build); };
+
     try {
         hook->machineName = readLine(hook->fromHook.readSide.get());
     } catch (Error & e) {
@@ -688,7 +692,7 @@ Goal::Co DerivationBuildingGoal::buildLocally(
 
                 void childTerminated() override
                 {
-                    goal.worker.childTerminated(&goal);
+                    goal.worker.childTerminated(&goal, JobCategory::Build);
                 }
 
                 void openLogFile() override
@@ -790,8 +794,7 @@ Goal::Co DerivationBuildingGoal::buildLocally(
             if (output->fd == builder->builderOut.get()) {
                 logSize += output->data.size();
                 if (settings.maxLogSize && logSize > settings.maxLogSize) {
-                    if (builder->killChild())
-                        worker.childTerminated(this);
+                    builder->killChild();
                     co_return doneFailureLogTooLong(*buildLog);
                 }
                 (*buildLog)(output->data);
@@ -802,8 +805,7 @@ Goal::Co DerivationBuildingGoal::buildLocally(
             buildLog->flush();
             break;
         } else if (auto * timeout = std::get_if<TimedOut>(&event)) {
-            if (builder->killChild())
-                worker.childTerminated(this);
+            builder->killChild();
             co_return doneFailure(std::move(*timeout));
         }
     }

src/libstore/build/substitution-goal.cc

@@ -324,7 +324,7 @@ void PathSubstitutionGoal::cleanup()
         if (thr.joinable()) {
             // FIXME: signal worker thread to quit.
             thr.join();
-            worker.childTerminated(this);
+            worker.childTerminated(this, JobCategory::Substitution);
         }
 
         outPipe.close();

src/libstore/build/worker.cc

@@ -162,7 +162,9 @@ template<typename G>
 static bool
 removeGoal(std::shared_ptr<G> goal, typename DerivedPathMap<std::map<OutputsSpec, std::weak_ptr<G>>>::ChildNode & node)
 {
-    return removeGoal(goal, node.value) || removeGoal(goal, node.childMap);
+    bool valueKeep = removeGoal(goal, node.value);
+    bool childMapKeep = removeGoal(goal, node.childMap);
+    return valueKeep || childMapKeep;
 }
 
 void Worker::removeGoal(GoalPtr goal)
@@ -245,13 +247,18 @@ void Worker::childStarted(
 }
 
 void Worker::childTerminated(Goal * goal, bool wakeSleepers)
+{
+    childTerminated(goal, goal->jobCategory(), wakeSleepers);
+}
+
+void Worker::childTerminated(Goal * goal, JobCategory jobCategory, bool wakeSleepers)
 {
     auto i = std::find_if(children.begin(), children.end(), [&](const Child & child) { return child.goal2 == goal; });
     if (i == children.end())
         return;
 
     if (i->inBuildSlot) {
-        switch (goal->jobCategory()) {
+        switch (jobCategory) {
         case JobCategory::Substitution:
             assert(nrSubstitutions > 0);
             nrSubstitutions--;

src/libstore/include/nix/store/build/worker.hh

@@ -295,9 +295,21 @@ public:
      * false if there is no sense in waking up goals that are sleeping
      * because they can't run yet (e.g., there is no free build slot,
      * or the hook would still say `postpone`).
+     *
+     * This overload requires `goal` to point to a fully constructed,
+     * valid goal object, as it calls `goal->jobCategory()`.
      */
     void childTerminated(Goal * goal, bool wakeSleepers = true);
 
+    /**
+     * Unregisters a running child process, like the other overload.
+     *
+     * This overload only uses `goal` as a pointer for comparison with
+     * weak goal references, so it is safe to call from destructors
+     * where the goal object may be partially destroyed.
+     */
+    void childTerminated(Goal * goal, JobCategory jobCategory, bool wakeSleepers = true);
+
     /**
      * Put `goal` to sleep until a build slot becomes available (which
      * might be right away).

src/libstore/unix/build/derivation-builder.cc

@@ -495,6 +495,8 @@ bool DerivationBuilderImpl::killChild()
         killSandbox(true);
 
         pid.wait();
+
+        miscMethods->childTerminated();
     }
     return ret;
 }

src/libstore/unix/build/hook-instance.cc

@@ -88,8 +88,11 @@ HookInstance::~HookInstance()
 {
     try {
         toHook.writeSide = -1;
-        if (pid != -1)
+        if (pid != -1) {
             pid.kill();
+            if (onKillChild)
+                onKillChild();
+        }
     } catch (...) {
         ignoreExceptionInDestructor();
     }

src/libstore/unix/include/nix/store/build/hook-instance.hh

@@ -5,6 +5,8 @@
 #include "nix/util/serialise.hh"
 #include "nix/util/processes.hh"
 
+#include <functional>
+
 namespace nix {
 
 /**
@@ -50,6 +52,12 @@ struct HookInstance
 
     std::map<ActivityId, Activity> activities;
 
+    /**
+     * Callback to run when the hook process is killed in the destructor.
+     * Used to call `Worker::childTerminated`.
+     */
+    std::function<void()> onKillChild;
+
     HookInstance();
 
     ~HookInstance();