libstore: move failingExitStatus into worker results

we already have a results type for the entire worker call, we may as
well put this bit of info in there. we do keep the assumption of the
old code that the field will only be read if some goals have failed;
fixing that is a very different mess, and not immediately necessary.

Change-Id: If3fc32649dcd88e1987cdd1758c6c5743e3b35ac

src/libstore/build/entry-points.cc

@@ -16,7 +16,7 @@ void Store::buildPaths(const std::vector<DerivedPath> & reqs, BuildMode buildMod
     auto aio = kj::setupAsyncIo();
     Worker worker(*this, evalStore ? *evalStore : *this, aio);
 
-    auto goals = runWorker(worker, [&](GoalFactory & gf) {
+    auto results = runWorker(worker, [&](GoalFactory & gf) {
         Worker::Targets goals;
         for (auto & br : reqs)
             goals.emplace(gf.makeGoal(br, buildMode));
@@ -25,7 +25,7 @@ void Store::buildPaths(const std::vector<DerivedPath> & reqs, BuildMode buildMod
 
     StringSet failed;
     std::shared_ptr<Error> ex;
-    for (auto & [i, result] : goals) {
+    for (auto & [i, result] : results.goals) {
         if (result.ex) {
             if (ex)
                 logError(result.ex->info());
@@ -41,11 +41,11 @@ void Store::buildPaths(const std::vector<DerivedPath> & reqs, BuildMode buildMod
     }
 
     if (failed.size() == 1 && ex) {
-        ex->withExitStatus(worker.failingExitStatus());
+        ex->withExitStatus(results.failingExitStatus);
         throw std::move(*ex);
     } else if (!failed.empty()) {
         if (ex) logError(ex->info());
-        throw Error(worker.failingExitStatus(), "build of %s failed", concatStringsSep(", ", quoteStrings(failed)));
+        throw Error(results.failingExitStatus, "build of %s failed", concatStringsSep(", ", quoteStrings(failed)));
     }
 }
 
@@ -67,7 +67,7 @@ std::vector<KeyedBuildResult> Store::buildPathsWithResults(
             goals.emplace(std::move(goal));
         }
         return goals;
-    });
+    }).goals;
 
     std::vector<KeyedBuildResult> results;
 
@@ -84,12 +84,12 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat
     Worker worker(*this, *this, aio);
 
     try {
-        auto goals = runWorker(worker, [&](GoalFactory & gf) {
+        auto results = runWorker(worker, [&](GoalFactory & gf) {
             Worker::Targets goals;
             goals.emplace(gf.makeBasicDerivationGoal(drvPath, drv, OutputsSpec::All{}, buildMode));
             return goals;
         });
-        auto [goal, result] = *goals.begin();
+        auto [goal, result] = *results.goals.begin();
         return result.result.restrictTo(DerivedPath::Built {
             .drvPath = makeConstantStorePathRef(drvPath),
             .outputs = OutputsSpec::All {},
@@ -111,19 +111,19 @@ void Store::ensurePath(const StorePath & path)
     auto aio = kj::setupAsyncIo();
     Worker worker(*this, *this, aio);
 
-    auto goals = runWorker(worker, [&](GoalFactory & gf) {
+    auto results = runWorker(worker, [&](GoalFactory & gf) {
         Worker::Targets goals;
         goals.emplace(gf.makePathSubstitutionGoal(path));
         return goals;
     });
-    auto [goal, result] = *goals.begin();
+    auto [goal, result] = *results.goals.begin();
 
     if (result.exitCode != Goal::ecSuccess) {
         if (result.ex) {
-            result.ex->withExitStatus(worker.failingExitStatus());
+            result.ex->withExitStatus(results.failingExitStatus);
             throw std::move(*result.ex);
         } else
-            throw Error(worker.failingExitStatus(), "path '%s' does not exist and cannot be created", printStorePath(path));
+            throw Error(results.failingExitStatus, "path '%s' does not exist and cannot be created", printStorePath(path));
     }
 }
 
@@ -133,12 +133,12 @@ void Store::repairPath(const StorePath & path)
     auto aio = kj::setupAsyncIo();
     Worker worker(*this, *this, aio);
 
-    auto goals = runWorker(worker, [&](GoalFactory & gf) {
+    auto results = runWorker(worker, [&](GoalFactory & gf) {
         Worker::Targets goals;
         goals.emplace(gf.makePathSubstitutionGoal(path, Repair));
         return goals;
     });
-    auto [goal, result] = *goals.begin();
+    auto [goal, result] = *results.goals.begin();
 
     if (result.exitCode != Goal::ecSuccess) {
         /* Since substituting the path didn't work, if we have a valid
@@ -158,7 +158,7 @@ void Store::repairPath(const StorePath & path)
                 return goals;
             });
         } else
-            throw Error(worker.failingExitStatus(), "cannot repair path '%s'", printStorePath(path));
+            throw Error(results.failingExitStatus, "cannot repair path '%s'", printStorePath(path));
     }
 }
 

src/libstore/build/worker.cc

@@ -270,7 +270,7 @@ try {
     while (auto done = co_await collect.next()) {
         // propagate goal exceptions outward
         BOOST_OUTCOME_CO_TRY(auto result, done->second);
-        results.emplace(done->first, result);
+        results.goals.emplace(done->first, result);
 
         /* If a top-level goal failed, then kill all other goals
            (unless keepGoing was set). */
@@ -285,24 +285,7 @@ try {
        --keep-going *is* set, then they must all be finished now. */
     assert(!settings.keepGoing || children.isEmpty());
 
-    co_return std::move(results);
+    results.failingExitStatus = [&] {
-} catch (...) {
-    co_return result::failure(std::current_exception());
-}
-
-kj::Promise<Result<Worker::Results>> Worker::boopGC(LocalStore & localStore)
-try {
-    while (true) {
-        co_await aio.provider->getTimer().afterDelay(10 * kj::SECONDS);
-        localStore.autoGC(false);
-    }
-} catch (...) {
-    co_return result::failure(std::current_exception());
-}
-
-
-unsigned int Worker::failingExitStatus()
-{
         // See API docs in header for explanation
         unsigned int mask = 0;
         bool buildFailure = permanentFailure || timedOut || hashMismatch;
@@ -319,6 +302,21 @@ unsigned int Worker::failingExitStatus()
         if (mask)
             mask |= 0x60;
         return mask ? mask : 1;
+    }();
+
+    co_return std::move(results);
+} catch (...) {
+    co_return result::failure(std::current_exception());
+}
+
+kj::Promise<Result<Worker::Results>> Worker::boopGC(LocalStore & localStore)
+try {
+    while (true) {
+        co_await aio.provider->getTimer().afterDelay(10 * kj::SECONDS);
+        localStore.autoGC(false);
+    }
+} catch (...) {
+    co_return result::failure(std::current_exception());
 }
 
 

src/libstore/build/worker.hh

@@ -86,7 +86,32 @@ class Worker : public WorkerBase
 {
 public:
     using Targets = std::map<GoalPtr, kj::Promise<Result<Goal::WorkResult>>>;
-    using Results = std::map<GoalPtr, Goal::WorkResult>;
+    struct Results {
+        std::map<GoalPtr, Goal::WorkResult> goals;
+
+        /**
+         * The exit status in case of failure.
+         *
+         * In the case of a build failure, returned value follows this
+         * bitmask:
+         *
+         * ```
+         * 0b1100100
+         *      ^^^^
+         *      |||`- timeout
+         *      ||`-- output hash mismatch
+         *      |`--- build failure
+         *      `---- not deterministic
+         * ```
+         *
+         * In other words, the failure code is at least 100 (0b1100100), but
+         * might also be greater.
+         *
+         * Otherwise (no build failure, but some other sort of failure by
+         * assumption), this returned value is 1.
+         */
+        unsigned int failingExitStatus;
+    };
 
 private:
 
@@ -248,29 +273,6 @@ public:
      */
     Results run(std::function<Targets (GoalFactory &)> req);
 
-    /***
-     * The exit status in case of failure.
-     *
-     * In the case of a build failure, returned value follows this
-     * bitmask:
-     *
-     * ```
-     * 0b1100100
-     *      ^^^^
-     *      |||`- timeout
-     *      ||`-- output hash mismatch
-     *      |`--- build failure
-     *      `---- not deterministic
-     * ```
-     *
-     * In other words, the failure code is at least 100 (0b1100100), but
-     * might also be greater.
-     *
-     * Otherwise (no build failure, but some other sort of failure by
-     * assumption), this returned value is 1.
-     */
-    unsigned int failingExitStatus();
-
     /**
      * Check whether the given valid path exists and has the right
      * contents.