Fix #9052

It's hard to reproduce this race condition going forward (e.g. it
depends on the precise order that goals are scheduled, but
https://github.com/roberth/nix-9052 current reproduces it, and this
fixes that.

Author: Ericson2314

src/libstore/build/derivation-creation-and-realisation-goal.cc

@@ -51,16 +51,23 @@ void DerivationCreationAndRealisationGoal::addWantedOutputs(const OutputsSpec &
 {
     /* If we already want all outputs, there is nothing to do. */
     auto newWanted = wantedOutputs.union_(outputs);
-    bool needRestart = !newWanted.isSubsetOf(wantedOutputs);
+    bool moreWanted = !newWanted.isSubsetOf(wantedOutputs);
     wantedOutputs = newWanted;
 
-    if (!needRestart)
+    if (!moreWanted)
         return;
 
     if (!optDrvPath)
         // haven't started steps where the outputs matter yet
         return;
-    worker.makeDerivationGoal(*optDrvPath, outputs, buildMode);
+    auto g = worker.makeDerivationGoal(*optDrvPath, outputs, buildMode);
+
+    if (!finalExitCode(upcast_goal(g)->exitCode)) {
+        /* The original concrete goal has already finished and been
+           destroyed, so reset this variable and double-suspend */
+        concreteDrvGoal = g;
+        needsRestart = true;
+    }
 }
 
 Goal::Co DerivationCreationAndRealisationGoal::init()
@@ -108,8 +115,12 @@ Goal::Co DerivationCreationAndRealisationGoal::init()
         g->preserveException = true;
     }
     optDrvPath = std::move(drvPath);
-    addWaitee(upcast_goal(concreteDrvGoal));
-    co_await Suspend{};
+
+    do {
+        needsRestart = false;
+        addWaitee(upcast_goal(concreteDrvGoal));
+        co_await Suspend{};
+    } while (needsRestart);
 
     trace("outer build done");
 

src/libstore/build/derivation-creation-and-realisation-goal.hh

@@ -47,6 +47,11 @@ struct DerivationCreationAndRealisationGoal : public Goal
      */
     OutputsSpec wantedOutputs;
 
+    /**
+     * Inner goal got recreated, need to wait again potentially.
+     */
+    bool needsRestart = false;
+
     /**
      * The final output paths of the build.
      *

src/libstore/build/goal.cc

@@ -166,12 +166,19 @@ void Goal::waiteeDone(GoalPtr waitee, ExitCode result)
     }
 }
 
+
+bool Goal::finalExitCode(ExitCode result)
+{
+    return result == ecSuccess || result == ecFailed || result == ecNoSubstituters || result == ecIncompleteClosure;
+}
+
+
 Goal::Done Goal::amDone(ExitCode result, std::optional<Error> ex)
 {
     trace("done");
     assert(top_co);
     assert(exitCode == ecBusy);
-    assert(result == ecSuccess || result == ecFailed || result == ecNoSubstituters || result == ecIncompleteClosure);
+    assert(finalExitCode(result));
     exitCode = result;
 
     if (ex) {

src/libstore/build/goal.hh

@@ -369,6 +369,18 @@ public:
      */
     Done amDone(ExitCode result, std::optional<Error> ex = {});
 
+    /**
+     * @return true just for those `ExitCode`s that are avalid argument
+     * to `amDone`.
+     *
+     * Used for an assert in `amDone`, and also in some not-so-pretty
+     * goal-retry logic.
+     *
+     * @todo If the latter caller is removed, then this function no
+     * longer needs to be exposed.
+     */
+    static bool finalExitCode(ExitCode result);
+
     virtual void cleanup() { }
 
     /**