Merge pull request #6312 from obsidiansystems/keyed-build-result

Shuffle `BuildResult` data definition, make state machine clearer, introduce `SingleDrvOutputs`

src/build-remote/build-remote.cc

@@ -311,8 +311,9 @@ connected:
                 auto thisOutputId = DrvOutput{ thisOutputHash, outputName };
                 if (!store->queryRealisation(thisOutputId)) {
                     debug("missing output %s", outputName);
-                    assert(result.builtOutputs.count(thisOutputId));
+                    auto i = result.builtOutputs.find(outputName);
-                    auto newRealisation = result.builtOutputs.at(thisOutputId);
+                    assert(i != result.builtOutputs.end());
+                    auto & newRealisation = i->second;
                     missingRealisations.insert(newRealisation);
                     missingPaths.insert(newRealisation.outPath);
                 }

src/libcmd/installables.cc

@@ -593,8 +593,8 @@ std::vector<std::pair<ref<Installable>, BuiltPathWithResult>> Installable::build
                 std::visit(overloaded {
                     [&](const DerivedPath::Built & bfd) {
                         std::map<std::string, StorePath> outputs;
-                        for (auto & path : buildResult.builtOutputs)
+                        for (auto & [outputName, realisation] : buildResult.builtOutputs)
-                            outputs.emplace(path.first.outputName, path.second.outPath);
+                            outputs.emplace(outputName, realisation.outPath);
                         res.push_back({aux.installable, {
                             .path = BuiltPath::Built { bfd.drvPath, outputs },
                             .info = aux.info,

src/libstore/build-result.hh

@@ -83,16 +83,11 @@ struct BuildResult
      */
     bool isNonDeterministic = false;
 
-    /**
-     * The derivation we built or the store path we substituted.
-     */
-    DerivedPath path;
-
     /**
      * For derivations, a mapping from the names of the wanted outputs
      * to actual paths.
      */
-    DrvOutputs builtOutputs;
+    SingleDrvOutputs builtOutputs;
 
     /**
      * The start/stop times of the build (or one of the rounds, if it
@@ -116,4 +111,15 @@ struct BuildResult
     }
 };
 
+/**
+ * A `BuildResult` together with its "primary key".
+ */
+struct KeyedBuildResult : BuildResult
+{
+    /**
+     * The derivation we built or the store path we substituted.
+     */
+    DerivedPath path;
+};
+
 }

src/libstore/build/derivation-goal.cc

@@ -145,8 +145,20 @@ void DerivationGoal::work()
 void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
 {
     auto newWanted = wantedOutputs.union_(outputs);
-    if (!newWanted.isSubsetOf(wantedOutputs))
+    switch (needRestart) {
-        needRestart = true;
+    case NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed:
+        if (!newWanted.isSubsetOf(wantedOutputs))
+            needRestart = NeedRestartForMoreOutputs::OutputsAddedDoNeed;
+        break;
+    case NeedRestartForMoreOutputs::OutputsAddedDoNeed:
+        /* No need to check whether we added more outputs, because a
+           restart is already queued up. */
+        break;
+    case NeedRestartForMoreOutputs::BuildInProgressWillNotNeed:
+        /* We are already building all outputs, so it doesn't matter if
+           we now want more. */
+        break;
+    };
     wantedOutputs = newWanted;
 }
 
@@ -297,12 +309,29 @@ void DerivationGoal::outputsSubstitutionTried()
         In particular, it may be the case that the hole in the closure is
         an output of the current derivation, which causes a loop if retried.
      */
-    if (nrIncompleteClosure > 0 && nrIncompleteClosure == nrFailed) retrySubstitution = true;
+    {
+        bool substitutionFailed =
+            nrIncompleteClosure > 0 &&
+            nrIncompleteClosure == nrFailed;
+        switch (retrySubstitution) {
+        case RetrySubstitution::NoNeed:
+            if (substitutionFailed)
+                retrySubstitution = RetrySubstitution::YesNeed;
+            break;
+        case RetrySubstitution::YesNeed:
+            // Should not be able to reach this state from here.
+            assert(false);
+            break;
+        case RetrySubstitution::AlreadyRetried:
+            debug("substitution failed again, but we already retried once. Not retrying again.");
+            break;
+        }
+    }
 
     nrFailed = nrNoSubstituters = nrIncompleteClosure = 0;
 
-    if (needRestart) {
+    if (needRestart == NeedRestartForMoreOutputs::OutputsAddedDoNeed) {
-        needRestart = false;
+        needRestart = NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed;
         haveDerivation();
         return;
     }
@@ -330,6 +359,10 @@ void DerivationGoal::outputsSubstitutionTried()
    produced using a substitute.  So we have to build instead. */
 void DerivationGoal::gaveUpOnSubstitution()
 {
+    /* At this point we are building all outputs, so if more are wanted there
+       is no need to restart. */
+    needRestart = NeedRestartForMoreOutputs::BuildInProgressWillNotNeed;
+
     /* The inputs must be built before we can build this goal. */
     inputDrvOutputs.clear();
     if (useDerivation)
@@ -451,8 +484,8 @@ void DerivationGoal::inputsRealised()
         return;
     }
 
-    if (retrySubstitution && !retriedSubstitution) {
+    if (retrySubstitution == RetrySubstitution::YesNeed) {
-        retriedSubstitution = true;
+        retrySubstitution = RetrySubstitution::AlreadyRetried;
         haveDerivation();
         return;
     }
@@ -570,8 +603,6 @@ void DerivationGoal::inputsRealised()
        build hook. */
     state = &DerivationGoal::tryToBuild;
     worker.wakeUp(shared_from_this());
-
-    buildResult = BuildResult { .path = buildResult.path };
 }
 
 void DerivationGoal::started()
@@ -982,7 +1013,7 @@ void DerivationGoal::resolvedFinished()
     auto resolvedDrv = *resolvedDrvGoal->drv;
     auto & resolvedResult = resolvedDrvGoal->buildResult;
 
-    DrvOutputs builtOutputs;
+    SingleDrvOutputs builtOutputs;
 
     if (resolvedResult.success()) {
         auto resolvedHashes = staticOutputHashes(worker.store, resolvedDrv);
@@ -1008,7 +1039,7 @@ void DerivationGoal::resolvedFinished()
                     worker.store.printStorePath(drvPath), wantedOutput);
 
             auto realisation = [&]{
-              auto take1 = get(resolvedResult.builtOutputs, DrvOutput { *resolvedHash, wantedOutput });
+              auto take1 = get(resolvedResult.builtOutputs, wantedOutput);
               if (take1) return *take1;
 
               /* The above `get` should work. But sateful tracking of
@@ -1033,7 +1064,7 @@ void DerivationGoal::resolvedFinished()
                 worker.store.registerDrvOutput(newRealisation);
             }
             outputPaths.insert(realisation.outPath);
-            builtOutputs.emplace(realisation.id, realisation);
+            builtOutputs.emplace(wantedOutput, realisation);
         }
 
         runPostBuildHook(
@@ -1158,7 +1189,7 @@ HookReply DerivationGoal::tryBuildHook()
 }
 
 
-DrvOutputs DerivationGoal::registerOutputs()
+SingleDrvOutputs DerivationGoal::registerOutputs()
 {
     /* When using a build hook, the build hook can register the output
        as valid (by doing `nix-store --import').  If so we don't have
@@ -1320,7 +1351,7 @@ OutputPathMap DerivationGoal::queryDerivationOutputMap()
 }
 
 
-std::pair<bool, DrvOutputs> DerivationGoal::checkPathValidity()
+std::pair<bool, SingleDrvOutputs> DerivationGoal::checkPathValidity()
 {
     if (!drv->type().isPure()) return { false, {} };
 
@@ -1333,7 +1364,7 @@ std::pair<bool, DrvOutputs> DerivationGoal::checkPathValidity()
             return static_cast<StringSet>(names);
         },
     }, wantedOutputs.raw());
-    DrvOutputs validOutputs;
+    SingleDrvOutputs validOutputs;
 
     for (auto & i : queryPartialDerivationOutputMap()) {
         auto initialOutput = get(initialOutputs, i.first);
@@ -1376,7 +1407,7 @@ std::pair<bool, DrvOutputs> DerivationGoal::checkPathValidity()
             }
         }
         if (info.wanted && info.known && info.known->isValid())
-            validOutputs.emplace(drvOutput, Realisation { drvOutput, info.known->path });
+            validOutputs.emplace(i.first, Realisation { drvOutput, info.known->path });
     }
 
     // If we requested all the outputs, we are always fine.
@@ -1400,7 +1431,7 @@ std::pair<bool, DrvOutputs> DerivationGoal::checkPathValidity()
 }
 
 
-DrvOutputs DerivationGoal::assertPathValidity()
+SingleDrvOutputs DerivationGoal::assertPathValidity()
 {
     auto [allValid, validOutputs] = checkPathValidity();
     if (!allValid)
@@ -1411,7 +1442,7 @@ DrvOutputs DerivationGoal::assertPathValidity()
 
 void DerivationGoal::done(
     BuildResult::Status status,
-    DrvOutputs builtOutputs,
+    SingleDrvOutputs builtOutputs,
     std::optional<Error> ex)
 {
     buildResult.status = status;
@@ -1452,12 +1483,28 @@ void DerivationGoal::waiteeDone(GoalPtr waitee, ExitCode result)
 {
     Goal::waiteeDone(waitee, result);
 
-    if (waitee->buildResult.success())
+    if (!useDerivation) return;
-        if (auto bfd = std::get_if<DerivedPath::Built>(&waitee->buildResult.path))
+    auto & fullDrv = *dynamic_cast<Derivation *>(drv.get());
-            for (auto & [output, realisation] : waitee->buildResult.builtOutputs)
+
+    auto * dg = dynamic_cast<DerivationGoal *>(&*waitee);
+    if (!dg) return;
+
+    auto outputs = fullDrv.inputDrvs.find(dg->drvPath);
+    if (outputs == fullDrv.inputDrvs.end()) return;
+
+    for (auto & outputName : outputs->second) {
+        auto buildResult = dg->getBuildResult(DerivedPath::Built {
+            .drvPath = dg->drvPath,
+            .outputs = OutputsSpec::Names { outputName },
+        });
+        if (buildResult.success()) {
+            auto i = buildResult.builtOutputs.find(outputName);
+            if (i != buildResult.builtOutputs.end())
                 inputDrvOutputs.insert_or_assign(
-                    { bfd->drvPath, output.outputName },
+                    { dg->drvPath, outputName },
-                    realisation.outPath);
+                    i->second.outPath);
+        }
+    }
 }
 
 }

src/libstore/build/derivation-goal.hh

@@ -78,22 +78,58 @@ struct DerivationGoal : public Goal
      */
     std::map<std::pair<StorePath, std::string>, StorePath> inputDrvOutputs;
 
+    /**
+     * See `needRestart`; just for that field.
+     */
+    enum struct NeedRestartForMoreOutputs {
+        /**
+         * The goal state machine is progressing based on the current value of
+         * `wantedOutputs. No actions are needed.
+         */
+        OutputsUnmodifedDontNeed,
+        /**
+         * `wantedOutputs` has been extended, but the state machine is
+         * proceeding according to its old value, so we need to restart.
+         */
+        OutputsAddedDoNeed,
+        /**
+         * The goal state machine has progressed to the point of doing a build,
+         * in which case all outputs will be produced, so extensions to
+         * `wantedOutputs` no longer require a restart.
+         */
+        BuildInProgressWillNotNeed,
+    };
+
     /**
      * Whether additional wanted outputs have been added.
      */
-    bool needRestart = false;
+    NeedRestartForMoreOutputs needRestart = NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed;
+
+    /**
+     * See `retrySubstitution`; just for that field.
+     */
+    enum RetrySubstitution {
+        /**
+         * No issues have yet arose, no need to restart.
+         */
+        NoNeed,
+        /**
+         * Something failed and there is an incomplete closure. Let's retry
+         * substituting.
+         */
+        YesNeed,
+        /**
+         * We are current or have already retried substitution, and whether or
+         * not something goes wrong we will not retry again.
+         */
+        AlreadyRetried,
+    };
 
     /**
      * Whether to retry substituting the outputs after building the
      * inputs. This is done in case of an incomplete closure.
      */
-    bool retrySubstitution = false;
+    RetrySubstitution retrySubstitution = RetrySubstitution::NoNeed;
-
-    /**
-     * Whether we've retried substitution, in which case we won't try
-     * again.
-     */
-    bool retriedSubstitution = false;
 
     /**
      * The derivation stored at drvPath.
@@ -217,7 +253,7 @@ struct DerivationGoal : public Goal
      * Check that the derivation outputs all exist and register them
      * as valid.
      */
-    virtual DrvOutputs registerOutputs();
+    virtual SingleDrvOutputs registerOutputs();
 
     /**
      * Open a log file and a pipe to it.
@@ -270,17 +306,17 @@ struct DerivationGoal : public Goal
      * Update 'initialOutputs' to determine the current status of the
      * outputs of the derivation. Also returns a Boolean denoting
      * whether all outputs are valid and non-corrupt, and a
-     * 'DrvOutputs' structure containing the valid and wanted
+     * 'SingleDrvOutputs' structure containing the valid and wanted
      * outputs.
      */
-    std::pair<bool, DrvOutputs> checkPathValidity();
+    std::pair<bool, SingleDrvOutputs> checkPathValidity();
 
     /**
      * Aborts if any output is not valid or corrupt, and otherwise
-     * returns a 'DrvOutputs' structure containing the wanted
+     * returns a 'SingleDrvOutputs' structure containing the wanted
      * outputs.
      */
-    DrvOutputs assertPathValidity();
+    SingleDrvOutputs assertPathValidity();
 
     /**
      * Forcibly kill the child process, if any.
@@ -293,7 +329,7 @@ struct DerivationGoal : public Goal
 
     void done(
         BuildResult::Status status,
-        DrvOutputs builtOutputs = {},
+        SingleDrvOutputs builtOutputs = {},
         std::optional<Error> ex = {});
 
     void waiteeDone(GoalPtr waitee, ExitCode result) override;

src/libstore/build/entry-points.cc

@@ -10,16 +10,8 @@ void Store::buildPaths(const std::vector<DerivedPath> & reqs, BuildMode buildMod
     Worker worker(*this, evalStore ? *evalStore : *this);
 
     Goals goals;
-    for (const auto & br : reqs) {
+    for (auto & br : reqs)
-        std::visit(overloaded {
+        goals.insert(worker.makeGoal(br, buildMode));
-            [&](const DerivedPath::Built & bfd) {
-                goals.insert(worker.makeDerivationGoal(bfd.drvPath, bfd.outputs, buildMode));
-            },
-            [&](const DerivedPath::Opaque & bo) {
-                goals.insert(worker.makePathSubstitutionGoal(bo.path, buildMode == bmRepair ? Repair : NoRepair));
-            },
-        }, br.raw());
-    }
 
     worker.run(goals);
 
@@ -47,7 +39,7 @@ void Store::buildPaths(const std::vector<DerivedPath> & reqs, BuildMode buildMod
     }
 }
 
-std::vector<BuildResult> Store::buildPathsWithResults(
+std::vector<KeyedBuildResult> Store::buildPathsWithResults(
     const std::vector<DerivedPath> & reqs,
     BuildMode buildMode,
     std::shared_ptr<Store> evalStore)
@@ -55,23 +47,23 @@ std::vector<BuildResult> Store::buildPathsWithResults(
     Worker worker(*this, evalStore ? *evalStore : *this);
 
     Goals goals;
-    for (const auto & br : reqs) {
+    std::vector<std::pair<const DerivedPath &, GoalPtr>> state;
-        std::visit(overloaded {
+
-            [&](const DerivedPath::Built & bfd) {
+    for (const auto & req : reqs) {
-                goals.insert(worker.makeDerivationGoal(bfd.drvPath, bfd.outputs, buildMode));
+        auto goal = worker.makeGoal(req, buildMode);
-            },
+        goals.insert(goal);
-            [&](const DerivedPath::Opaque & bo) {
+        state.push_back({req, goal});
-                goals.insert(worker.makePathSubstitutionGoal(bo.path, buildMode == bmRepair ? Repair : NoRepair));
-            },
-        }, br.raw());
     }
 
     worker.run(goals);
 
-    std::vector<BuildResult> results;
+    std::vector<KeyedBuildResult> results;
 
-    for (auto & i : goals)
+    for (auto & [req, goalPtr] : state)
-        results.push_back(i->buildResult);
+        results.emplace_back(KeyedBuildResult {
+            goalPtr->getBuildResult(req),
+            /* .path = */ req,
+        });
 
     return results;
 }
@@ -84,15 +76,14 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat
 
     try {
         worker.run(Goals{goal});
-        return goal->buildResult;
+        return goal->getBuildResult(DerivedPath::Built {
+            .drvPath = drvPath,
+            .outputs = OutputsSpec::All {},
+        });
     } catch (Error & e) {
         return BuildResult {
             .status = BuildResult::MiscFailure,
             .errorMsg = e.msg(),
-            .path = DerivedPath::Built {
-                .drvPath = drvPath,
-                .outputs = OutputsSpec::All { },
-            },
         };
     };
 }

src/libstore/build/goal.cc

@@ -11,6 +11,29 @@ bool CompareGoalPtrs::operator() (const GoalPtr & a, const GoalPtr & b) const {
 }
 
 
+BuildResult Goal::getBuildResult(const DerivedPath & req) {
+    BuildResult res { buildResult };
+
+    if (auto pbp = std::get_if<DerivedPath::Built>(&req)) {
+        auto & bp = *pbp;
+
+        /* Because goals are in general shared between derived paths
+           that share the same derivation, we need to filter their
+           results to get back just the results we care about.
+         */
+
+        for (auto it = res.builtOutputs.begin(); it != res.builtOutputs.end();) {
+            if (bp.outputs.contains(it->first))
+                ++it;
+            else
+                it = res.builtOutputs.erase(it);
+        }
+    }
+
+    return res;
+}
+
+
 void addToWeakGoals(WeakGoals & goals, GoalPtr p)
 {
     if (goals.find(p) != goals.end())

src/libstore/build/goal.hh

@@ -81,11 +81,26 @@ struct Goal : public std::enable_shared_from_this<Goal>
      */
     ExitCode exitCode = ecBusy;
 
+protected:
     /**
      * Build result.
      */
     BuildResult buildResult;
 
+public:
+
+    /**
+     * Project a `BuildResult` with just the information that pertains
+     * to the given request.
+     *
+     * In general, goals may be aliased between multiple requests, and
+     * the stored `BuildResult` has information for the union of all
+     * requests. We don't want to leak what the other request are for
+     * sake of both privacy and determinism, and this "safe accessor"
+     * ensures we don't.
+     */
+    BuildResult getBuildResult(const DerivedPath &);
+
     /**
      * Exception containing an error message, if any.
      */
@@ -93,7 +108,6 @@ struct Goal : public std::enable_shared_from_this<Goal>
 
     Goal(Worker & worker, DerivedPath path)
         : worker(worker)
-        , buildResult { .path = std::move(path) }
     { }
 
     virtual ~Goal()

src/libstore/build/local-derivation-goal.cc

@@ -1335,7 +1335,7 @@ struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual Lo
                 result.rethrow();
     }
 
-    std::vector<BuildResult> buildPathsWithResults(
+    std::vector<KeyedBuildResult> buildPathsWithResults(
         const std::vector<DerivedPath> & paths,
         BuildMode buildMode = bmNormal,
         std::shared_ptr<Store> evalStore = nullptr) override
@@ -2174,7 +2174,7 @@ void LocalDerivationGoal::runChild()
 }
 
 
-DrvOutputs LocalDerivationGoal::registerOutputs()
+SingleDrvOutputs LocalDerivationGoal::registerOutputs()
 {
     /* When using a build hook, the build hook can register the output
        as valid (by doing `nix-store --import').  If so we don't have
@@ -2683,7 +2683,7 @@ DrvOutputs LocalDerivationGoal::registerOutputs()
        means it's safe to link the derivation to the output hash. We must do
        that for floating CA derivations, which otherwise couldn't be cached,
        but it's fine to do in all cases. */
-    DrvOutputs builtOutputs;
+    SingleDrvOutputs builtOutputs;
 
     for (auto & [outputName, newInfo] : infos) {
         auto oldinfo = get(initialOutputs, outputName);
@@ -2702,7 +2702,7 @@ DrvOutputs LocalDerivationGoal::registerOutputs()
             worker.store.registerDrvOutput(thisRealisation);
         }
         if (wantedOutputs.contains(outputName))
-            builtOutputs.emplace(thisRealisation.id, thisRealisation);
+            builtOutputs.emplace(outputName, thisRealisation);
     }
 
     return builtOutputs;

src/libstore/build/local-derivation-goal.hh

@@ -237,7 +237,7 @@ struct LocalDerivationGoal : public DerivationGoal
      * Check that the derivation outputs all exist and register them
      * as valid.
      */
-    DrvOutputs registerOutputs() override;
+    SingleDrvOutputs registerOutputs() override;
 
     void signRealisation(Realisation &) override;
 

src/libstore/build/worker.cc

@@ -92,6 +92,7 @@ std::shared_ptr<PathSubstitutionGoal> Worker::makePathSubstitutionGoal(const Sto
     return goal;
 }
 
+
 std::shared_ptr<DrvOutputSubstitutionGoal> Worker::makeDrvOutputSubstitutionGoal(const DrvOutput& id, RepairFlag repair, std::optional<ContentAddress> ca)
 {
     std::weak_ptr<DrvOutputSubstitutionGoal> & goal_weak = drvOutputSubstitutionGoals[id];
@@ -104,6 +105,20 @@ std::shared_ptr<DrvOutputSubstitutionGoal> Worker::makeDrvOutputSubstitutionGoal
     return goal;
 }
 
+
+GoalPtr Worker::makeGoal(const DerivedPath & req, BuildMode buildMode)
+{
+    return std::visit(overloaded {
+        [&](const DerivedPath::Built & bfd) -> GoalPtr {
+            return makeDerivationGoal(bfd.drvPath, bfd.outputs, buildMode);
+        },
+        [&](const DerivedPath::Opaque & bo) -> GoalPtr {
+            return makePathSubstitutionGoal(bo.path, buildMode == bmRepair ? Repair : NoRepair);
+        },
+    }, req.raw());
+}
+
+
 template<typename K, typename G>
 static void removeGoal(std::shared_ptr<G> goal, std::map<K, std::weak_ptr<G>> & goalMap)
 {

src/libstore/build/worker.hh

@@ -181,7 +181,7 @@ public:
      */
 
     /**
-     * derivation goal
+     * @ref DerivationGoal "derivation goal"
      */
 private:
     std::shared_ptr<DerivationGoal> makeDerivationGoalCommon(
@@ -196,11 +196,19 @@ public:
         const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal);
 
     /**
-     * substitution goal
+     * @ref SubstitutionGoal "substitution goal"
      */
     std::shared_ptr<PathSubstitutionGoal> makePathSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt);
     std::shared_ptr<DrvOutputSubstitutionGoal> makeDrvOutputSubstitutionGoal(const DrvOutput & id, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt);
 
+    /**
+     * Make a goal corresponding to the `DerivedPath`.
+     *
+     * It will be a `DerivationGoal` for a `DerivedPath::Built` or
+     * a `SubstitutionGoal` for a `DerivedPath::Opaque`.
+     */
+    GoalPtr makeGoal(const DerivedPath & req, BuildMode buildMode = bmNormal);
+
     /**
      * Remove a dead goal.
      */

src/libstore/daemon.cc

@@ -637,7 +637,10 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
             to << res.timesBuilt << res.isNonDeterministic << res.startTime << res.stopTime;
         }
         if (GET_PROTOCOL_MINOR(clientVersion) >= 28) {
-            worker_proto::write(*store, to, res.builtOutputs);
+            DrvOutputs builtOutputs;
+            for (auto & [output, realisation] : res.builtOutputs)
+                builtOutputs.insert_or_assign(realisation.id, realisation);
+            worker_proto::write(*store, to, builtOutputs);
         }
         break;
     }

src/libstore/legacy-ssh-store.cc

@@ -287,19 +287,18 @@ public:
 
         conn->to.flush();
 
-        BuildResult status {
+        BuildResult status;
-            .path = DerivedPath::Built {
-                .drvPath = drvPath,
-                .outputs = OutputsSpec::All { },
-            },
-        };
         status.status = (BuildResult::Status) readInt(conn->from);
         conn->from >> status.errorMsg;
 
         if (GET_PROTOCOL_MINOR(conn->remoteVersion) >= 3)
             conn->from >> status.timesBuilt >> status.isNonDeterministic >> status.startTime >> status.stopTime;
         if (GET_PROTOCOL_MINOR(conn->remoteVersion) >= 6) {
-            status.builtOutputs = worker_proto::read(*this, conn->from, Phantom<DrvOutputs> {});
+            auto builtOutputs = worker_proto::read(*this, conn->from, Phantom<DrvOutputs> {});
+            for (auto && [output, realisation] : builtOutputs)
+                status.builtOutputs.insert_or_assign(
+                    std::move(output.outputName),
+                    std::move(realisation));
         }
         return status;
     }
@@ -330,7 +329,7 @@ public:
 
         conn->to.flush();
 
-        BuildResult result { .path = DerivedPath::Opaque { StorePath::dummy } };
+        BuildResult result;
         result.status = (BuildResult::Status) readInt(conn->from);
 
         if (!result.success()) {

src/libstore/realisation.hh

@@ -13,9 +13,25 @@ namespace nix {
 
 class Store;
 
+/**
+ * A general `Realisation` key.
+ *
+ * This is similar to a `DerivedPath::Opaque`, but the derivation is
+ * identified by its "hash modulo" instead of by its store path.
+ */
 struct DrvOutput {
-    // The hash modulo of the derivation
+    /**
+     * The hash modulo of the derivation.
+     *
+     * Computed from the derivation itself for most types of
+     * derivations, but computed from the (fixed) content address of the
+     * output for fixed-output derivations.
+     */
     Hash drvHash;
+
+    /**
+     * The name of the output.
+     */
     std::string outputName;
 
     std::string to_string() const;
@@ -60,6 +76,21 @@ struct Realisation {
     GENERATE_CMP(Realisation, me->id, me->outPath);
 };
 
+/**
+ * Collection type for a single derivation's outputs' `Realisation`s.
+ *
+ * Since these are the outputs of a single derivation, we know the
+ * output names are unique so we can use them as the map key.
+ */
+typedef std::map<std::string, Realisation> SingleDrvOutputs;
+
+/**
+ * Collection type for multiple derivations' outputs' `Realisation`s.
+ *
+ * `DrvOutput` is used because in general the derivations are not all
+ * the same, so we need to identify firstly which derivation, and
+ * secondly which output of that derivation.
+ */
 typedef std::map<DrvOutput, Realisation> DrvOutputs;
 
 struct OpaquePath {

src/libstore/remote-store.cc

@@ -125,10 +125,26 @@ void write(const Store & store, Sink & out, const DrvOutput & drvOutput)
 }
 
 
-BuildResult read(const Store & store, Source & from, Phantom<BuildResult> _)
+KeyedBuildResult read(const Store & store, Source & from, Phantom<KeyedBuildResult> _)
 {
     auto path = worker_proto::read(store, from, Phantom<DerivedPath> {});
-    BuildResult res { .path = path };
+    auto br = worker_proto::read(store, from, Phantom<BuildResult> {});
+    return KeyedBuildResult {
+        std::move(br),
+        /* .path = */ std::move(path),
+    };
+}
+
+void write(const Store & store, Sink & to, const KeyedBuildResult & res)
+{
+    worker_proto::write(store, to, res.path);
+    worker_proto::write(store, to, static_cast<const BuildResult &>(res));
+}
+
+
+BuildResult read(const Store & store, Source & from, Phantom<BuildResult> _)
+{
+    BuildResult res;
     res.status = (BuildResult::Status) readInt(from);
     from
         >> res.errorMsg
@@ -136,13 +152,16 @@ BuildResult read(const Store & store, Source & from, Phantom<BuildResult> _)
         >> res.isNonDeterministic
         >> res.startTime
         >> res.stopTime;
-    res.builtOutputs = worker_proto::read(store, from, Phantom<DrvOutputs> {});
+    auto builtOutputs = worker_proto::read(store, from, Phantom<DrvOutputs> {});
+    for (auto && [output, realisation] : builtOutputs)
+        res.builtOutputs.insert_or_assign(
+            std::move(output.outputName),
+            std::move(realisation));
     return res;
 }
 
 void write(const Store & store, Sink & to, const BuildResult & res)
 {
-    worker_proto::write(store, to, res.path);
     to
         << res.status
         << res.errorMsg
@@ -150,7 +169,10 @@ void write(const Store & store, Sink & to, const BuildResult & res)
         << res.isNonDeterministic
         << res.startTime
         << res.stopTime;
-    worker_proto::write(store, to, res.builtOutputs);
+    DrvOutputs builtOutputs;
+    for (auto & [output, realisation] : res.builtOutputs)
+        builtOutputs.insert_or_assign(realisation.id, realisation);
+    worker_proto::write(store, to, builtOutputs);
 }
 
 
@@ -865,7 +887,7 @@ void RemoteStore::buildPaths(const std::vector<DerivedPath> & drvPaths, BuildMod
     readInt(conn->from);
 }
 
-std::vector<BuildResult> RemoteStore::buildPathsWithResults(
+std::vector<KeyedBuildResult> RemoteStore::buildPathsWithResults(
     const std::vector<DerivedPath> & paths,
     BuildMode buildMode,
     std::shared_ptr<Store> evalStore)
@@ -880,7 +902,7 @@ std::vector<BuildResult> RemoteStore::buildPathsWithResults(
         writeDerivedPaths(*this, conn, paths);
         conn->to << buildMode;
         conn.processStderr();
-        return worker_proto::read(*this, conn->from, Phantom<std::vector<BuildResult>> {});
+        return worker_proto::read(*this, conn->from, Phantom<std::vector<KeyedBuildResult>> {});
     } else {
         // Avoid deadlock.
         conn_.reset();
@@ -889,21 +911,25 @@ std::vector<BuildResult> RemoteStore::buildPathsWithResults(
         // fails, but meh.
         buildPaths(paths, buildMode, evalStore);
 
-        std::vector<BuildResult> results;
+        std::vector<KeyedBuildResult> results;
 
         for (auto & path : paths) {
             std::visit(
                 overloaded {
                     [&](const DerivedPath::Opaque & bo) {
-                        results.push_back(BuildResult {
+                        results.push_back(KeyedBuildResult {
-                            .status = BuildResult::Substituted,
+                            {
-                            .path = bo,
+                                .status = BuildResult::Substituted,
+                            },
+                            /* .path = */ bo,
                         });
                     },
                     [&](const DerivedPath::Built & bfd) {
-                        BuildResult res {
+                        KeyedBuildResult res {
-                            .status = BuildResult::Built,
+                            {
-                            .path = bfd,
+                                .status = BuildResult::Built
+                            },
+                            /* .path = */ bfd,
                         };
 
                         OutputPathMap outputs;
@@ -922,10 +948,10 @@ std::vector<BuildResult> RemoteStore::buildPathsWithResults(
                                     queryRealisation(outputId);
                                 if (!realisation)
                                     throw MissingRealisation(outputId);
-                                res.builtOutputs.emplace(realisation->id, *realisation);
+                                res.builtOutputs.emplace(output, *realisation);
                             } else {
                                 res.builtOutputs.emplace(
-                                    outputId,
+                                    output,
                                     Realisation {
                                         .id = outputId,
                                         .outPath = outputPath,
@@ -952,12 +978,7 @@ BuildResult RemoteStore::buildDerivation(const StorePath & drvPath, const BasicD
     writeDerivation(conn->to, *this, drv);
     conn->to << buildMode;
     conn.processStderr();
-    BuildResult res {
+    BuildResult res;
-        .path = DerivedPath::Built {
-            .drvPath = drvPath,
-            .outputs = OutputsSpec::All { },
-        },
-    };
     res.status = (BuildResult::Status) readInt(conn->from);
     conn->from >> res.errorMsg;
     if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 29) {
@@ -965,7 +986,10 @@ BuildResult RemoteStore::buildDerivation(const StorePath & drvPath, const BasicD
     }
     if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 28) {
         auto builtOutputs = worker_proto::read(*this, conn->from, Phantom<DrvOutputs> {});
-        res.builtOutputs = builtOutputs;
+        for (auto && [output, realisation] : builtOutputs)
+            res.builtOutputs.insert_or_assign(
+                std::move(output.outputName),
+                std::move(realisation));
     }
     return res;
 }

src/libstore/remote-store.hh

@@ -114,7 +114,7 @@ public:
 
     void buildPaths(const std::vector<DerivedPath> & paths, BuildMode buildMode, std::shared_ptr<Store> evalStore) override;
 
-    std::vector<BuildResult> buildPathsWithResults(
+    std::vector<KeyedBuildResult> buildPathsWithResults(
         const std::vector<DerivedPath> & paths,
         BuildMode buildMode,
         std::shared_ptr<Store> evalStore) override;

src/libstore/store-api.hh

@@ -92,6 +92,7 @@ enum BuildMode { bmNormal, bmRepair, bmCheck };
 enum TrustedFlag : bool { NotTrusted = false, Trusted = true };
 
 struct BuildResult;
+struct KeyedBuildResult;
 
 
 typedef std::map<StorePath, std::optional<ContentAddress>> StorePathCAMap;
@@ -569,7 +570,7 @@ public:
      * case of a build/substitution error, this function won't throw an
      * exception, but return a BuildResult containing an error message.
      */
-    virtual std::vector<BuildResult> buildPathsWithResults(
+    virtual std::vector<KeyedBuildResult> buildPathsWithResults(
         const std::vector<DerivedPath> & paths,
         BuildMode buildMode = bmNormal,
         std::shared_ptr<Store> evalStore = nullptr);

src/libstore/worker-protocol.hh

@@ -103,6 +103,7 @@ MAKE_WORKER_PROTO(, DerivedPath);
 MAKE_WORKER_PROTO(, Realisation);
 MAKE_WORKER_PROTO(, DrvOutput);
 MAKE_WORKER_PROTO(, BuildResult);
+MAKE_WORKER_PROTO(, KeyedBuildResult);
 MAKE_WORKER_PROTO(, std::optional<TrustedFlag>);
 
 MAKE_WORKER_PROTO(template<typename T>, std::vector<T>);

src/nix-store/nix-store.cc

@@ -941,7 +941,10 @@ static void opServe(Strings opFlags, Strings opArgs)
                 if (GET_PROTOCOL_MINOR(clientVersion) >= 3)
                     out << status.timesBuilt << status.isNonDeterministic << status.startTime << status.stopTime;
                 if (GET_PROTOCOL_MINOR(clientVersion) >= 6) {
-                    worker_proto::write(*store, out, status.builtOutputs);
+                    DrvOutputs builtOutputs;
+                    for (auto & [output, realisation] : status.builtOutputs)
+                        builtOutputs.insert_or_assign(realisation.id, realisation);
+                    worker_proto::write(*store, out, builtOutputs);
                 }
 
                 break;