From 5b1715e63349541c1d021f6426b2ad67a0bf518f Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Wed, 2 Oct 2024 15:39:17 +0200 Subject: [PATCH] libstore: forbid addWantedGoals when finished due to event loop scheduling behavior it's possible for a derivation goal to fully finish (having seen all paths it was asked to create), but to not notify the worker of this in time to prevent another goal asking the recently-finished goal for more outputs. if this happened the finished goal would ignore the request for more outputs since it considered itself fully done, and the delayed result reporting would cause the requesting goal to assume its request had been honored. if the requested goal had finished *properly* the worker would recreate it instead of asking for more outputs, and this would succeed. it is thus safe to always recreate goals once they are done, so we now do. Change-Id: Ifedd69ca153372c623abe9a9b49cd1523588814f --- src/libstore/build/derivation-goal.cc | 9 ++++- src/libstore/build/derivation-goal.hh | 8 ++++- src/libstore/build/worker.cc | 47 +++++++++++++++++---------- src/libstore/build/worker.hh | 2 +- 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> 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 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 @@ -73,6 +73,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. */ @@ -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, kj::Promise> Worker::makeGoalCommon( std::map> & map, const ID & key, InvocableR> auto create, - std::invocable auto modify + InvocableR 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, kj::Promise> Worker::makeDeriva drvPath, wantedOutputs, *this, running, buildMode ); }, - [&](DerivationGoal & g) { g.addWantedOutputs(wantedOutputs); } + [&](DerivationGoal & g) { return g.addWantedOutputs(wantedOutputs); } ); } @@ -118,7 +129,7 @@ std::pair, kj::Promise> 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(path, *this, running, repair, ca); }, - [&](auto &) {} + [&](auto &) { return true; } ); } @@ -146,7 +157,7 @@ Worker::makeDrvOutputSubstitutionGoal( drvOutputSubstitutionGoals, id, [&] { return std::make_unique(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> & map, const ID & key, InvocableR> auto create, - std::invocable auto modify + InvocableR auto modify ); std::pair, kj::Promise> makeDerivationGoal( const StorePath & drvPath,