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
This commit is contained in:
eldritch horrors 2024-10-02 15:39:17 +02:00
parent 0b29859cfe
commit 5b1715e633
4 changed files with 45 additions and 21 deletions

View file

@ -130,8 +130,12 @@ kj::Promise<Result<Goal::WorkResult>> DerivationGoal::work() noexcept
return useDerivation ? getDerivation() : haveDerivation(); 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); auto newWanted = wantedOutputs.union_(outputs);
switch (needRestart) { switch (needRestart) {
case NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed: case NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed:
@ -148,6 +152,7 @@ void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
break; break;
}; };
wantedOutputs = newWanted; wantedOutputs = newWanted;
return true;
} }
@ -1680,6 +1685,8 @@ Goal::Finished DerivationGoal::done(
SingleDrvOutputs builtOutputs, SingleDrvOutputs builtOutputs,
std::optional<Error> ex) std::optional<Error> ex)
{ {
isDone = true;
outputLocks.unlock(); outputLocks.unlock();
buildResult.status = status; buildResult.status = status;
if (ex) if (ex)

View file

@ -73,6 +73,12 @@ struct DerivationGoal : public Goal
{ {
struct InputStream; 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. * 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. * Add wanted outputs to an already existing derivation goal.
*/ */
void addWantedOutputs(const OutputsSpec & outputs); bool addWantedOutputs(const OutputsSpec & outputs);
/** /**
* The states. * The states.

View file

@ -57,24 +57,35 @@ std::pair<std::shared_ptr<G>, kj::Promise<void>> Worker::makeGoalCommon(
std::map<ID, CachedGoal<G>> & map, std::map<ID, CachedGoal<G>> & map,
const ID & key, const ID & key,
InvocableR<std::unique_ptr<G>> auto create, 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 [it, _inserted] = map.try_emplace(key);
auto & goal_weak = it->second; // try twice to create the goal. we can only loop if we hit the continue,
auto goal = goal_weak.goal.lock(); // and then we only want to recreate the goal *once*. concurrent accesses
if (!goal) { // to the worker are not sound, we want to catch them if at all possible.
goal = create(); for ([[maybe_unused]] auto _attempt : {1, 2}) {
goal->notify = std::move(goal_weak.fulfiller); auto & goal_weak = it->second;
goal_weak.goal = goal; auto goal = goal_weak.goal.lock();
// do not start working immediately, this round of the event loop if (!goal) {
// may have more calls to this function lined up that'll also run goal = create();
// modify(). starting early can then cause the goals to misbehave goal->notify = std::move(goal_weak.fulfiller);
childStarted(goal, kj::evalLater([goal] { return goal->work(); })); goal_weak.goal = goal;
} else { // do not start working immediately. if we are not yet running we
modify(*goal); // 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 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 drvPath, drv, wantedOutputs, *this, running, buildMode
); );
}, },
[&](DerivationGoal & g) { g.addWantedOutputs(wantedOutputs); } [&](DerivationGoal & g) { return g.addWantedOutputs(wantedOutputs); }
); );
} }
@ -132,7 +143,7 @@ Worker::makePathSubstitutionGoal(
substitutionGoals, substitutionGoals,
path, path,
[&] { return std::make_unique<PathSubstitutionGoal>(path, *this, running, repair, ca); }, [&] { return std::make_unique<PathSubstitutionGoal>(path, *this, running, repair, ca); },
[&](auto &) {} [&](auto &) { return true; }
); );
} }
@ -146,7 +157,7 @@ Worker::makeDrvOutputSubstitutionGoal(
drvOutputSubstitutionGoals, drvOutputSubstitutionGoals,
id, id,
[&] { return std::make_unique<DrvOutputSubstitutionGoal>(id, *this, running, repair, ca); }, [&] { return std::make_unique<DrvOutputSubstitutionGoal>(id, *this, running, repair, ca); },
[&](auto &) {} [&](auto &) { return true; }
); );
} }

View file

@ -241,7 +241,7 @@ private:
std::map<ID, CachedGoal<G>> & map, std::map<ID, CachedGoal<G>> & map,
const ID & key, const ID & key,
InvocableR<std::unique_ptr<G>> auto create, 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( std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> makeDerivationGoal(
const StorePath & drvPath, const StorePath & drvPath,