4 files changed, 45 insertions, 21 deletions

diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc
index 5c0452391..d90a8639d 100644
--- a/src/libstore/build/derivation-goal.cc
+++ b/src/libstore/build/derivation-goal.cc
@@ -130,8 +130,12 @@ kj::Promise<Result<Goal::WorkResult>> DerivationGoal::work() noexcept
     return useDerivation ? getDerivation() : haveDerivation();
 }
 
-void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
+bool DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
 {
+    if (isDone) {
+        return false;
+    }
+
     auto newWanted = wantedOutputs.union_(outputs);
     switch (needRestart) {
     case NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed:
@@ -148,6 +152,7 @@ void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
         break;
     };
     wantedOutputs = newWanted;
+    return true;
 }
 
 
@@ -1680,6 +1685,8 @@ Goal::Finished DerivationGoal::done(
     SingleDrvOutputs builtOutputs,
     std::optional<Error> ex)
 {
+    isDone = true;
+
     outputLocks.unlock();
     buildResult.status = status;
     if (ex)
diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh
index 7505409c0..1e2f9b55c 100644
--- a/src/libstore/build/derivation-goal.hh
+++ b/src/libstore/build/derivation-goal.hh
@@ -74,6 +74,12 @@ struct DerivationGoal : public Goal
     struct InputStream;
 
     /**
+      * Whether this goal has completed. Completed goals can not be
+      * asked for more outputs, a new goal must be created instead.
+      */
+    bool isDone = false;
+
+    /**
      * Whether to use an on-disk .drv file.
      */
     bool useDerivation;
@@ -249,7 +255,7 @@ struct DerivationGoal : public Goal
     /**
      * Add wanted outputs to an already existing derivation goal.
      */
-    void addWantedOutputs(const OutputsSpec & outputs);
+    bool addWantedOutputs(const OutputsSpec & outputs);
 
     /**
      * The states.
diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc
index 5be706e42..e063ede71 100644
--- a/src/libstore/build/worker.cc
+++ b/src/libstore/build/worker.cc
@@ -57,24 +57,35 @@ std::pair<std::shared_ptr<G>, kj::Promise<void>> Worker::makeGoalCommon(
     std::map<ID, CachedGoal<G>> & map,
     const ID & key,
     InvocableR<std::unique_ptr<G>> auto create,
-    std::invocable<G &> auto modify
+    InvocableR<bool, G &> auto modify
 )
 {
     auto [it, _inserted] = map.try_emplace(key);
-    auto & goal_weak = it->second;
-    auto goal = goal_weak.goal.lock();
-    if (!goal) {
-        goal = create();
-        goal->notify = std::move(goal_weak.fulfiller);
-        goal_weak.goal = goal;
-        // do not start working immediately, this round of the event loop
-        // may have more calls to this function lined up that'll also run
-        // modify(). starting early can then cause the goals to misbehave
-        childStarted(goal, kj::evalLater([goal] { return goal->work(); }));
-    } else {
-        modify(*goal);
+    // try twice to create the goal. we can only loop if we hit the continue,
+    // and then we only want to recreate the goal *once*. concurrent accesses
+    // to the worker are not sound, we want to catch them if at all possible.
+    for ([[maybe_unused]] auto _attempt : {1, 2}) {
+        auto & goal_weak = it->second;
+        auto goal = goal_weak.goal.lock();
+        if (!goal) {
+            goal = create();
+            goal->notify = std::move(goal_weak.fulfiller);
+            goal_weak.goal = goal;
+            // do not start working immediately. if we are not yet running we
+            // may create dependencies as though they were toplevel goals, in
+            // which case the dependencies will not report build errors. when
+            // we are running we may be called for this same goal more times,
+            // and then we want to modify rather than recreate when possible.
+            childStarted(goal, kj::evalLater([goal] { return goal->work(); }));
+        } else {
+            if (!modify(*goal)) {
+                goal_weak = {};
+                continue;
+            }
+        }
+        return {goal, goal_weak.promise->addBranch()};
     }
-    return {goal, goal_weak.promise->addBranch()};
+    assert(false && "could not make a goal. possible concurrent worker access");
 }
 
 
@@ -94,7 +105,7 @@ std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> Worker::makeDeriva
                     drvPath, wantedOutputs, *this, running, buildMode
                 );
         },
-        [&](DerivationGoal & g) { g.addWantedOutputs(wantedOutputs); }
+        [&](DerivationGoal & g) { return g.addWantedOutputs(wantedOutputs); }
     );
 }
 
@@ -118,7 +129,7 @@ std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> Worker::makeBasicD
                     drvPath, drv, wantedOutputs, *this, running, buildMode
                 );
         },
-        [&](DerivationGoal & g) { g.addWantedOutputs(wantedOutputs); }
+        [&](DerivationGoal & g) { return g.addWantedOutputs(wantedOutputs); }
     );
 }
 
@@ -132,7 +143,7 @@ Worker::makePathSubstitutionGoal(
         substitutionGoals,
         path,
         [&] { return std::make_unique<PathSubstitutionGoal>(path, *this, running, repair, ca); },
-        [&](auto &) {}
+        [&](auto &) { return true; }
     );
 }
 
@@ -146,7 +157,7 @@ Worker::makeDrvOutputSubstitutionGoal(
         drvOutputSubstitutionGoals,
         id,
         [&] { return std::make_unique<DrvOutputSubstitutionGoal>(id, *this, running, repair, ca); },
-        [&](auto &) {}
+        [&](auto &) { return true; }
     );
 }
 
diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh
index d6cde8384..cd49fb860 100644
--- a/src/libstore/build/worker.hh
+++ b/src/libstore/build/worker.hh
@@ -241,7 +241,7 @@ private:
         std::map<ID, CachedGoal<G>> & map,
         const ID & key,
         InvocableR<std::unique_ptr<G>> auto create,
-        std::invocable<G &> auto modify
+        InvocableR<bool, G &> auto modify
     );
     std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> makeDerivationGoal(
         const StorePath & drvPath,