Merge pull request #14060 from obsidiansystems/build-result-variant

Use `std::variant` to enforce `BuildResult` invariants

src/libcmd/installables.cc

@@ -604,28 +604,28 @@ std::vector<BuiltPathWithResult> Installable::build(
 
 static void throwBuildErrors(std::vector<KeyedBuildResult> & buildResults, const Store & store)
 {
-    std::vector<KeyedBuildResult> failed;
+    std::vector<std::pair<const KeyedBuildResult *, const KeyedBuildResult::Failure *>> failed;
     for (auto & buildResult : buildResults) {
-        if (!buildResult.success()) {
+        if (auto * failure = buildResult.tryGetFailure()) {
-            failed.push_back(buildResult);
+            failed.push_back({&buildResult, failure});
         }
     }
 
     auto failedResult = failed.begin();
     if (failedResult != failed.end()) {
         if (failed.size() == 1) {
-            failedResult->rethrow();
+            failedResult->second->rethrow();
         } else {
             StringSet failedPaths;
             for (; failedResult != failed.end(); failedResult++) {
-                if (!failedResult->errorMsg.empty()) {
+                if (!failedResult->second->errorMsg.empty()) {
                     logError(
                         ErrorInfo{
                             .level = lvlError,
-                            .msg = failedResult->errorMsg,
+                            .msg = failedResult->second->errorMsg,
                         });
                 }
-                failedPaths.insert(failedResult->path.to_string(store));
+                failedPaths.insert(failedResult->first->path.to_string(store));
             }
             throw Error("build of %s failed", concatStringsSep(", ", quoteStrings(failedPaths)));
         }
@@ -695,12 +695,14 @@ std::vector<std::pair<ref<Installable>, BuiltPathWithResult>> Installable::build
         auto buildResults = store->buildPathsWithResults(pathsToBuild, bMode, evalStore);
         throwBuildErrors(buildResults, *store);
         for (auto & buildResult : buildResults) {
+            // If we didn't throw, they must all be sucesses
+            auto & success = std::get<nix::BuildResult::Success>(buildResult.inner);
             for (auto & aux : backmap[buildResult.path]) {
                 std::visit(
                     overloaded{
                         [&](const DerivedPath::Built & bfd) {
                             std::map<std::string, StorePath> outputs;
-                            for (auto & [outputName, realisation] : buildResult.builtOutputs)
+                            for (auto & [outputName, realisation] : success.builtOutputs)
                                 outputs.emplace(outputName, realisation.outPath);
                             res.push_back(
                                 {aux.installable,

src/libstore-c/nix_api_store.cc

@@ -145,13 +145,15 @@ nix_err nix_store_realise(
 
         if (callback) {
             for (const auto & result : results) {
-                for (const auto & [outputName, realisation] : result.builtOutputs) {
+                if (auto * success = result.tryGetSuccess()) {
+                    for (const auto & [outputName, realisation] : success->builtOutputs) {
                         StorePath p{realisation.outPath};
                         callback(userdata, outputName.c_str(), &p);
                     }
                 }
             }
         }
+    }
     NIXC_CATCH_ERRS
 }
 

src/libstore-tests/serve-protocol.cc

@@ -127,17 +127,17 @@ VERSIONED_CHARACTERIZATION_TEST(
 VERSIONED_CHARACTERIZATION_TEST(ServeProtoTest, buildResult_2_2, "build-result-2.2", 2 << 8 | 2, ({
                                     using namespace std::literals::chrono_literals;
                                     std::tuple<BuildResult, BuildResult, BuildResult> t{
-                                        BuildResult{
+                                        BuildResult{.inner{BuildResult::Failure{
-                                            .status = BuildResult::OutputRejected,
+                                            .status = BuildResult::Failure::OutputRejected,
                                             .errorMsg = "no idea why",
-                                        },
+                                        }}},
-                                        BuildResult{
+                                        BuildResult{.inner{BuildResult::Failure{
-                                            .status = BuildResult::NotDeterministic,
+                                            .status = BuildResult::Failure::NotDeterministic,
                                             .errorMsg = "no idea why",
-                                        },
+                                        }}},
-                                        BuildResult{
+                                        BuildResult{.inner{BuildResult::Success{
-                                            .status = BuildResult::Built,
+                                            .status = BuildResult::Success::Built,
-                                        },
+                                        }}},
                                     };
                                     t;
                                 }))
@@ -145,20 +145,24 @@ VERSIONED_CHARACTERIZATION_TEST(ServeProtoTest, buildResult_2_2, "build-result-2
 VERSIONED_CHARACTERIZATION_TEST(ServeProtoTest, buildResult_2_3, "build-result-2.3", 2 << 8 | 3, ({
                                     using namespace std::literals::chrono_literals;
                                     std::tuple<BuildResult, BuildResult, BuildResult> t{
-                                        BuildResult{
+                                        BuildResult{.inner{BuildResult::Failure{
-                                            .status = BuildResult::OutputRejected,
+                                            .status = BuildResult::Failure::OutputRejected,
                                             .errorMsg = "no idea why",
-                                        },
+                                        }}},
                                         BuildResult{
-                                            .status = BuildResult::NotDeterministic,
+                                            .inner{BuildResult::Failure{
+                                                .status = BuildResult::Failure::NotDeterministic,
                                                 .errorMsg = "no idea why",
-                                            .timesBuilt = 3,
                                                 .isNonDeterministic = true,
+                                            }},
+                                            .timesBuilt = 3,
                                             .startTime = 30,
                                             .stopTime = 50,
                                         },
                                         BuildResult{
-                                            .status = BuildResult::Built,
+                                            .inner{BuildResult::Success{
+                                                .status = BuildResult::Success::Built,
+                                            }},
                                             .startTime = 30,
                                             .stopTime = 50,
                                         },
@@ -170,21 +174,23 @@ VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest, buildResult_2_6, "build-result-2.6", 2 << 8 | 6, ({
         using namespace std::literals::chrono_literals;
         std::tuple<BuildResult, BuildResult, BuildResult> t{
-            BuildResult{
+            BuildResult{.inner{BuildResult::Failure{
-                .status = BuildResult::OutputRejected,
+                .status = BuildResult::Failure::OutputRejected,
                 .errorMsg = "no idea why",
-            },
+            }}},
             BuildResult{
-                .status = BuildResult::NotDeterministic,
+                .inner{BuildResult::Failure{
+                    .status = BuildResult::Failure::NotDeterministic,
                     .errorMsg = "no idea why",
-                .timesBuilt = 3,
                     .isNonDeterministic = true,
+                }},
+                .timesBuilt = 3,
                 .startTime = 30,
                 .stopTime = 50,
             },
             BuildResult{
-                .status = BuildResult::Built,
+                .inner{BuildResult::Success{
-                .timesBuilt = 1,
+                    .status = BuildResult::Success::Built,
                     .builtOutputs =
                         {
                             {
@@ -212,6 +218,8 @@ VERSIONED_CHARACTERIZATION_TEST(
                                 },
                             },
                         },
+                }},
+                .timesBuilt = 1,
                 .startTime = 30,
                 .stopTime = 50,
 #if 0

src/libstore-tests/worker-protocol.cc

@@ -180,17 +180,17 @@ VERSIONED_CHARACTERIZATION_TEST(
 VERSIONED_CHARACTERIZATION_TEST(WorkerProtoTest, buildResult_1_27, "build-result-1.27", 1 << 8 | 27, ({
                                     using namespace std::literals::chrono_literals;
                                     std::tuple<BuildResult, BuildResult, BuildResult> t{
-                                        BuildResult{
+                                        BuildResult{.inner{BuildResult::Failure{
-                                            .status = BuildResult::OutputRejected,
+                                            .status = BuildResult::Failure::OutputRejected,
                                             .errorMsg = "no idea why",
-                                        },
+                                        }}},
-                                        BuildResult{
+                                        BuildResult{.inner{BuildResult::Failure{
-                                            .status = BuildResult::NotDeterministic,
+                                            .status = BuildResult::Failure::NotDeterministic,
                                             .errorMsg = "no idea why",
-                                        },
+                                        }}},
-                                        BuildResult{
+                                        BuildResult{.inner{BuildResult::Success{
-                                            .status = BuildResult::Built,
+                                            .status = BuildResult::Success::Built,
-                                        },
+                                        }}},
                                     };
                                     t;
                                 }))
@@ -199,16 +199,16 @@ VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest, buildResult_1_28, "build-result-1.28", 1 << 8 | 28, ({
         using namespace std::literals::chrono_literals;
         std::tuple<BuildResult, BuildResult, BuildResult> t{
-            BuildResult{
+            BuildResult{.inner{BuildResult::Failure{
-                .status = BuildResult::OutputRejected,
+                .status = BuildResult::Failure::OutputRejected,
                 .errorMsg = "no idea why",
-            },
+            }}},
-            BuildResult{
+            BuildResult{.inner{BuildResult::Failure{
-                .status = BuildResult::NotDeterministic,
+                .status = BuildResult::Failure::NotDeterministic,
                 .errorMsg = "no idea why",
-            },
+            }}},
-            BuildResult{
+            BuildResult{.inner{BuildResult::Success{
-                .status = BuildResult::Built,
+                .status = BuildResult::Success::Built,
                 .builtOutputs =
                     {
                         {
@@ -236,7 +236,7 @@ VERSIONED_CHARACTERIZATION_TEST(
                             },
                         },
                     },
-            },
+            }}},
         };
         t;
     }))
@@ -245,21 +245,23 @@ VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest, buildResult_1_29, "build-result-1.29", 1 << 8 | 29, ({
         using namespace std::literals::chrono_literals;
         std::tuple<BuildResult, BuildResult, BuildResult> t{
-            BuildResult{
+            BuildResult{.inner{BuildResult::Failure{
-                .status = BuildResult::OutputRejected,
+                .status = BuildResult::Failure::OutputRejected,
                 .errorMsg = "no idea why",
-            },
+            }}},
             BuildResult{
-                .status = BuildResult::NotDeterministic,
+                .inner{BuildResult::Failure{
+                    .status = BuildResult::Failure::NotDeterministic,
                     .errorMsg = "no idea why",
-                .timesBuilt = 3,
                     .isNonDeterministic = true,
+                }},
+                .timesBuilt = 3,
                 .startTime = 30,
                 .stopTime = 50,
             },
             BuildResult{
-                .status = BuildResult::Built,
+                .inner{BuildResult::Success{
-                .timesBuilt = 1,
+                    .status = BuildResult::Success::Built,
                     .builtOutputs =
                         {
                             {
@@ -287,6 +289,8 @@ VERSIONED_CHARACTERIZATION_TEST(
                                 },
                             },
                         },
+                }},
+                .timesBuilt = 1,
                 .startTime = 30,
                 .stopTime = 50,
             },
@@ -298,21 +302,23 @@ VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest, buildResult_1_37, "build-result-1.37", 1 << 8 | 37, ({
         using namespace std::literals::chrono_literals;
         std::tuple<BuildResult, BuildResult, BuildResult> t{
-            BuildResult{
+            BuildResult{.inner{BuildResult::Failure{
-                .status = BuildResult::OutputRejected,
+                .status = BuildResult::Failure::OutputRejected,
                 .errorMsg = "no idea why",
-            },
+            }}},
             BuildResult{
-                .status = BuildResult::NotDeterministic,
+                .inner{BuildResult::Failure{
+                    .status = BuildResult::Failure::NotDeterministic,
                     .errorMsg = "no idea why",
-                .timesBuilt = 3,
                     .isNonDeterministic = true,
+                }},
+                .timesBuilt = 3,
                 .startTime = 30,
                 .stopTime = 50,
             },
             BuildResult{
-                .status = BuildResult::Built,
+                .inner{BuildResult::Success{
-                .timesBuilt = 1,
+                    .status = BuildResult::Success::Built,
                     .builtOutputs =
                         {
                             {
@@ -340,6 +346,8 @@ VERSIONED_CHARACTERIZATION_TEST(
                                 },
                             },
                         },
+                }},
+                .timesBuilt = 1,
                 .startTime = 30,
                 .stopTime = 50,
                 .cpuUser = std::chrono::microseconds(500s),
@@ -353,10 +361,10 @@ VERSIONED_CHARACTERIZATION_TEST(WorkerProtoTest, keyedBuildResult_1_29, "keyed-b
                                     using namespace std::literals::chrono_literals;
                                     std::tuple<KeyedBuildResult, KeyedBuildResult /*, KeyedBuildResult*/> t{
                                         KeyedBuildResult{
-                                            {
+                                            {.inner{BuildResult::Failure{
-                                                .status = KeyedBuildResult::OutputRejected,
+                                                .status = KeyedBuildResult::Failure::OutputRejected,
                                                 .errorMsg = "no idea why",
-                                            },
+                                            }}},
                                             /* .path = */
                                             DerivedPath::Opaque{
                                                 StorePath{"g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-xxx"},
@@ -364,10 +372,12 @@ VERSIONED_CHARACTERIZATION_TEST(WorkerProtoTest, keyedBuildResult_1_29, "keyed-b
                                         },
                                         KeyedBuildResult{
                                             {
-                                                .status = KeyedBuildResult::NotDeterministic,
+                                                .inner{BuildResult::Failure{
+                                                    .status = KeyedBuildResult::Failure::NotDeterministic,
                                                     .errorMsg = "no idea why",
-                                                .timesBuilt = 3,
                                                     .isNonDeterministic = true,
+                                                }},
+                                                .timesBuilt = 3,
                                                 .startTime = 30,
                                                 .stopTime = 50,
                                             },

src/libstore/build-result.cc

@@ -5,4 +5,10 @@ namespace nix {
 bool BuildResult::operator==(const BuildResult &) const noexcept = default;
 std::strong_ordering BuildResult::operator<=>(const BuildResult &) const noexcept = default;
 
+bool BuildResult::Success::operator==(const BuildResult::Success &) const noexcept = default;
+std::strong_ordering BuildResult::Success::operator<=>(const BuildResult::Success &) const noexcept = default;
+
+bool BuildResult::Failure::operator==(const BuildResult::Failure &) const noexcept = default;
+std::strong_ordering BuildResult::Failure::operator<=>(const BuildResult::Failure &) const noexcept = default;
+
 } // namespace nix

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

@@ -90,7 +90,7 @@ void DerivationBuildingGoal::timedOut(Error && ex)
     killChild();
     // We're not inside a coroutine, hence we can't use co_return here.
     // Thus we ignore the return value.
-    [[maybe_unused]] Done _ = doneFailure({BuildResult::TimedOut, std::move(ex)});
+    [[maybe_unused]] Done _ = doneFailure({BuildResult::Failure::TimedOut, std::move(ex)});
 }
 
 /**
@@ -205,7 +205,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
                 nrFailed,
                 nrFailed == 1 ? "dependency" : "dependencies");
         msg += showKnownOutputs(worker.store, *drv);
-        co_return doneFailure(BuildError(BuildResult::DependencyFailed, msg));
+        co_return doneFailure(BuildError(BuildResult::Failure::DependencyFailed, msg));
     }
 
     /* Gather information necessary for computing the closure and/or
@@ -256,14 +256,18 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
                         return std::nullopt;
 
                     auto & buildResult = (*mEntry)->buildResult;
-                    if (!buildResult.success())
+                    return std::visit(
-                        return std::nullopt;
+                        overloaded{
-
+                            [](const BuildResult::Failure &) -> std::optional<StorePath> { return std::nullopt; },
-                    auto i = get(buildResult.builtOutputs, outputName);
+                            [&](const BuildResult::Success & success) -> std::optional<StorePath> {
+                                auto i = get(success.builtOutputs, outputName);
                                 if (!i)
                                     return std::nullopt;
 
                                 return i->outPath;
+                            },
+                        },
+                        buildResult.inner);
                 });
             if (!attempt) {
                 /* TODO (impure derivations-induced tech debt) (see below):
@@ -306,7 +310,9 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
 
             auto resolvedResult = resolvedDrvGoal->buildResult;
 
-            if (resolvedResult.success()) {
+            // No `std::visit` for coroutines yet
+            if (auto * successP = resolvedResult.tryGetSuccess()) {
+                auto & success = *successP;
                 SingleDrvOutputs builtOutputs;
 
                 auto outputHashes = staticOutputHashes(worker.evalStore, *drv);
@@ -324,7 +330,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
                             outputName);
 
                     auto realisation = [&] {
-                        auto take1 = get(resolvedResult.builtOutputs, outputName);
+                        auto take1 = get(success.builtOutputs, outputName);
                         if (take1)
                             return *take1;
 
@@ -360,18 +366,19 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
 
                 runPostBuildHook(worker.store, *logger, drvPath, outputPaths);
 
-                auto status = resolvedResult.status;
+                auto status = success.status;
-                if (status == BuildResult::AlreadyValid)
+                if (status == BuildResult::Success::AlreadyValid)
-                    status = BuildResult::ResolvesToAlreadyValid;
+                    status = BuildResult::Success::ResolvesToAlreadyValid;
 
-                co_return doneSuccess(status, std::move(builtOutputs));
+                co_return doneSuccess(success.status, std::move(builtOutputs));
-            } else {
+            } else if (resolvedResult.tryGetFailure()) {
                 co_return doneFailure({
-                    BuildResult::DependencyFailed,
+                    BuildResult::Failure::DependencyFailed,
                     "build of resolved derivation '%s' failed",
                     worker.store.printStorePath(pathResolved),
                 });
-            }
+            } else
+                assert(false);
         }
 
         /* If we get this far, we know no dynamic drvs inputs */
@@ -536,7 +543,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
             debug("skipping build of derivation '%s', someone beat us to it", worker.store.printStorePath(drvPath));
             outputLocks.setDeletion(true);
             outputLocks.unlock();
-            co_return doneSuccess(BuildResult::AlreadyValid, std::move(validOutputs));
+            co_return doneSuccess(BuildResult::Success::AlreadyValid, std::move(validOutputs));
         }
 
         /* If any of the outputs already exist but are not valid, delete
@@ -628,7 +635,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
 
         /* Check the exit status. */
         if (!statusOk(status)) {
-            auto e = fixupBuilderFailureErrorMessage({BuildResult::MiscFailure, status, ""});
+            auto e = fixupBuilderFailureErrorMessage({BuildResult::Failure::MiscFailure, status, ""});
 
             outputLocks.unlock();
 
@@ -669,7 +676,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
         outputLocks.setDeletion(true);
         outputLocks.unlock();
 
-        co_return doneSuccess(BuildResult::Built, std::move(builtOutputs));
+        co_return doneSuccess(BuildResult::Success::Built, std::move(builtOutputs));
     }
 
     co_await yield();
@@ -832,15 +839,15 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
 #  pragma GCC diagnostic push
 #  pragma GCC diagnostic ignored "-Wswitch-enum"
         switch (e.status) {
-        case BuildResult::HashMismatch:
+        case BuildResult::Failure::HashMismatch:
             worker.hashMismatch = true;
             /* See header, the protocols don't know about `HashMismatch`
                yet, so change it to `OutputRejected`, which they expect
                for this case (hash mismatch is a type of output
                rejection). */
-            e.status = BuildResult::OutputRejected;
+            e.status = BuildResult::Failure::OutputRejected;
             break;
-        case BuildResult::NotDeterministic:
+        case BuildResult::Failure::NotDeterministic:
             worker.checkMismatch = true;
             break;
         default:
@@ -866,7 +873,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
            (unlinked) lock files. */
         outputLocks.setDeletion(true);
         outputLocks.unlock();
-        co_return doneSuccess(BuildResult::Built, std::move(builtOutputs));
+        co_return doneSuccess(BuildResult::Success::Built, std::move(builtOutputs));
     }
 #endif
 }
@@ -1149,7 +1156,7 @@ void DerivationBuildingGoal::handleChildOutput(Descriptor fd, std::string_view d
             // We're not inside a coroutine, hence we can't use co_return here.
             // Thus we ignore the return value.
             [[maybe_unused]] Done _ = doneFailure(BuildError(
-                BuildResult::LogLimitExceeded,
+                BuildResult::Failure::LogLimitExceeded,
                 "%s killed after writing more than %d bytes of log output",
                 getName(),
                 settings.maxLogSize));
@@ -1306,16 +1313,16 @@ DerivationBuildingGoal::checkPathValidity(std::map<std::string, InitialOutput> &
     return {allValid, validOutputs};
 }
 
-Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Status status, SingleDrvOutputs builtOutputs)
+Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Success::Status status, SingleDrvOutputs builtOutputs)
 {
-    buildResult.status = status;
+    buildResult.inner = BuildResult::Success{
-
+        .status = status,
-    assert(buildResult.success());
+        .builtOutputs = std::move(builtOutputs),
+    };
 
     mcRunningBuilds.reset();
 
-    buildResult.builtOutputs = std::move(builtOutputs);
+    if (status == BuildResult::Success::Built)
-    if (status == BuildResult::Built)
         worker.doneBuilds++;
 
     worker.updateProgress();
@@ -1325,16 +1332,18 @@ Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Status status, Singl
 
 Goal::Done DerivationBuildingGoal::doneFailure(BuildError ex)
 {
-    buildResult.status = ex.status;
+    buildResult.inner = BuildResult::Failure{
-    buildResult.errorMsg = fmt("%s", Uncolored(ex.info().msg));
+        .status = ex.status,
-    if (buildResult.status == BuildResult::TimedOut)
+        .errorMsg = fmt("%s", Uncolored(ex.info().msg)),
-        worker.timedOut = true;
+    };
-    if (buildResult.status == BuildResult::PermanentFailure)
-        worker.permanentFailure = true;
 
     mcRunningBuilds.reset();
 
-    if (ex.status != BuildResult::DependencyFailed)
+    if (ex.status == BuildResult::Failure::TimedOut)
+        worker.timedOut = true;
+    if (ex.status == BuildResult::Failure::PermanentFailure)
+        worker.permanentFailure = true;
+    if (ex.status != BuildResult::Failure::DependencyFailed)
         worker.failedBuilds++;
 
     worker.updateProgress();

src/libstore/build/derivation-check.cc

@@ -33,7 +33,7 @@ void checkOutputs(
                 /* Throw an error after registering the path as
                    valid. */
                 throw BuildError(
-                    BuildResult::HashMismatch,
+                    BuildResult::Failure::HashMismatch,
                     "hash mismatch in fixed-output derivation '%s':\n  specified: %s\n     got:    %s",
                     store.printStorePath(drvPath),
                     wanted.to_string(HashFormat::SRI, true),
@@ -42,7 +42,7 @@ void checkOutputs(
             if (!info.references.empty()) {
                 auto numViolations = info.references.size();
                 throw BuildError(
-                    BuildResult::HashMismatch,
+                    BuildResult::Failure::HashMismatch,
                     "fixed-output derivations must not reference store paths: '%s' references %d distinct paths, e.g. '%s'",
                     store.printStorePath(drvPath),
                     numViolations,
@@ -84,7 +84,7 @@ void checkOutputs(
         auto applyChecks = [&](const DerivationOptions::OutputChecks & checks) {
             if (checks.maxSize && info.narSize > *checks.maxSize)
                 throw BuildError(
-                    BuildResult::OutputRejected,
+                    BuildResult::Failure::OutputRejected,
                     "path '%s' is too large at %d bytes; limit is %d bytes",
                     store.printStorePath(info.path),
                     info.narSize,
@@ -94,7 +94,7 @@ void checkOutputs(
                 uint64_t closureSize = getClosure(info.path).second;
                 if (closureSize > *checks.maxClosureSize)
                     throw BuildError(
-                        BuildResult::OutputRejected,
+                        BuildResult::Failure::OutputRejected,
                         "closure of path '%s' is too large at %d bytes; limit is %d bytes",
                         store.printStorePath(info.path),
                         closureSize,
@@ -115,7 +115,7 @@ void checkOutputs(
                         std::string outputsListing =
                             concatMapStringsSep(", ", outputs, [](auto & o) { return o.first; });
                         throw BuildError(
-                            BuildResult::OutputRejected,
+                            BuildResult::Failure::OutputRejected,
                             "derivation '%s' output check for '%s' contains an illegal reference specifier '%s',"
                             " expected store path or output name (one of [%s])",
                             store.printStorePath(drvPath),
@@ -148,7 +148,7 @@ void checkOutputs(
                         badPathsStr += store.printStorePath(i);
                     }
                     throw BuildError(
-                        BuildResult::OutputRejected,
+                        BuildResult::Failure::OutputRejected,
                         "output '%s' is not allowed to refer to the following paths:%s",
                         store.printStorePath(info.path),
                         badPathsStr);

src/libstore/build/derivation-goal.cc

@@ -94,7 +94,7 @@ Goal::Co DerivationGoal::haveDerivation()
 
         /* If they are all valid, then we're done. */
         if (checkResult && checkResult->second == PathStatus::Valid && buildMode == bmNormal) {
-            co_return doneSuccess(BuildResult::AlreadyValid, checkResult->first);
+            co_return doneSuccess(BuildResult::Success::AlreadyValid, checkResult->first);
         }
 
         Goals waitees;
@@ -123,7 +123,7 @@ Goal::Co DerivationGoal::haveDerivation()
 
         if (nrFailed > 0 && nrFailed > nrNoSubstituters && !settings.tryFallback) {
             co_return doneFailure(BuildError(
-                BuildResult::TransientFailure,
+                BuildResult::Failure::TransientFailure,
                 "some substitutes for the outputs of derivation '%s' failed (usually happens due to networking issues); try '--fallback' to build derivation from source ",
                 worker.store.printStorePath(drvPath)));
         }
@@ -135,7 +135,7 @@ Goal::Co DerivationGoal::haveDerivation()
         bool allValid = checkResult && checkResult->second == PathStatus::Valid;
 
         if (buildMode == bmNormal && allValid) {
-            co_return doneSuccess(BuildResult::Substituted, checkResult->first);
+            co_return doneSuccess(BuildResult::Success::Substituted, checkResult->first);
         }
         if (buildMode == bmRepair && allValid) {
             co_return repairClosure();
@@ -163,25 +163,27 @@ Goal::Co DerivationGoal::haveDerivation()
 
     buildResult = g->buildResult;
 
+    if (auto * successP = buildResult.tryGetSuccess()) {
+        auto & success = *successP;
         if (buildMode == bmCheck) {
             /* In checking mode, the builder will not register any outputs.
                So we want to make sure the ones that we wanted to check are
                properly there. */
-        buildResult.builtOutputs = {{wantedOutput, assertPathValidity()}};
+            success.builtOutputs = {{wantedOutput, assertPathValidity()}};
         } else {
             /* Otherwise the builder will give us info for out output, but
                also for other outputs. Filter down to just our output so as
                not to leak info on unrelated things. */
-        for (auto it = buildResult.builtOutputs.begin(); it != buildResult.builtOutputs.end();) {
+            for (auto it = success.builtOutputs.begin(); it != success.builtOutputs.end();) {
                 if (it->first != wantedOutput) {
-                it = buildResult.builtOutputs.erase(it);
+                    it = success.builtOutputs.erase(it);
                 } else {
                     ++it;
                 }
             }
 
-        if (buildResult.success())
+            assert(success.builtOutputs.count(wantedOutput) > 0);
-            assert(buildResult.builtOutputs.count(wantedOutput) > 0);
+        }
     }
 
     co_return amDone(g->exitCode, g->ex);
@@ -279,7 +281,7 @@ Goal::Co DerivationGoal::repairClosure()
                 "some paths in the output closure of derivation '%s' could not be repaired",
                 worker.store.printStorePath(drvPath));
     }
-    co_return doneSuccess(BuildResult::AlreadyValid, assertPathValidity());
+    co_return doneSuccess(BuildResult::Success::AlreadyValid, assertPathValidity());
 }
 
 std::optional<std::pair<Realisation, PathStatus>> DerivationGoal::checkPathValidity()
@@ -337,16 +339,16 @@ Realisation DerivationGoal::assertPathValidity()
     return checkResult->first;
 }
 
-Goal::Done DerivationGoal::doneSuccess(BuildResult::Status status, Realisation builtOutput)
+Goal::Done DerivationGoal::doneSuccess(BuildResult::Success::Status status, Realisation builtOutput)
 {
-    buildResult.status = status;
+    buildResult.inner = BuildResult::Success{
-
+        .status = status,
-    assert(buildResult.success());
+        .builtOutputs = {{wantedOutput, std::move(builtOutput)}},
+    };
 
     mcExpectedBuilds.reset();
 
-    buildResult.builtOutputs = {{wantedOutput, std::move(builtOutput)}};
+    if (status == BuildResult::Success::Built)
-    if (status == BuildResult::Built)
         worker.doneBuilds++;
 
     worker.updateProgress();
@@ -356,16 +358,18 @@ Goal::Done DerivationGoal::doneSuccess(BuildResult::Status status, Realisation b
 
 Goal::Done DerivationGoal::doneFailure(BuildError ex)
 {
-    buildResult.status = ex.status;
+    buildResult.inner = BuildResult::Failure{
-    buildResult.errorMsg = fmt("%s", Uncolored(ex.info().msg));
+        .status = ex.status,
-    if (buildResult.status == BuildResult::TimedOut)
+        .errorMsg = fmt("%s", Uncolored(ex.info().msg)),
-        worker.timedOut = true;
+    };
-    if (buildResult.status == BuildResult::PermanentFailure)
-        worker.permanentFailure = true;
 
     mcExpectedBuilds.reset();
 
-    if (ex.status != BuildResult::DependencyFailed)
+    if (ex.status == BuildResult::Failure::TimedOut)
+        worker.timedOut = true;
+    if (ex.status == BuildResult::Failure::PermanentFailure)
+        worker.permanentFailure = true;
+    if (ex.status != BuildResult::Failure::DependencyFailed)
         worker.failedBuilds++;
 
     worker.updateProgress();

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

@@ -164,10 +164,11 @@ Goal::Co DerivationTrampolineGoal::haveDerivation(StorePath drvPath, Derivation
 
     auto & g = *concreteDrvGoals.begin();
     buildResult = g->buildResult;
-    for (auto & g2 : concreteDrvGoals) {
+    if (auto * successP = buildResult.tryGetSuccess())
-        for (auto && [x, y] : g2->buildResult.builtOutputs)
+        for (auto & g2 : concreteDrvGoals)
-            buildResult.builtOutputs.insert_or_assign(x, y);
+            if (auto * successP2 = g2->buildResult.tryGetSuccess())
-    }
+                for (auto && [x, y] : successP2->builtOutputs)
+                    successP->builtOutputs.insert_or_assign(x, y);
 
     co_return amDone(g->exitCode, g->ex);
 }

src/libstore/build/entry-points.cc

@@ -82,10 +82,10 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat
         worker.run(Goals{goal});
         return goal->buildResult;
     } catch (Error & e) {
-        return BuildResult{
+        return BuildResult{.inner{BuildResult::Failure{
-            .status = BuildResult::MiscFailure,
+            .status = BuildResult::Failure::MiscFailure,
             .errorMsg = e.msg(),
-        };
+        }}};
     };
 }
 

src/libstore/build/substitution-goal.cc

@@ -27,13 +27,21 @@ PathSubstitutionGoal::~PathSubstitutionGoal()
     cleanup();
 }
 
-Goal::Done PathSubstitutionGoal::done(ExitCode result, BuildResult::Status status, std::optional<std::string> errorMsg)
+Goal::Done PathSubstitutionGoal::doneSuccess(BuildResult::Success::Status status)
 {
-    buildResult.status = status;
+    buildResult.inner = BuildResult::Success{
-    if (errorMsg) {
+        .status = status,
-        debug(*errorMsg);
+    };
-        buildResult.errorMsg = *errorMsg;
+    return amDone(ecSuccess);
-    }
+}
+
+Goal::Done PathSubstitutionGoal::doneFailure(ExitCode result, BuildResult::Failure::Status status, std::string errorMsg)
+{
+    debug(errorMsg);
+    buildResult.inner = BuildResult::Failure{
+        .status = status,
+        .errorMsg = std::move(errorMsg),
+    };
     return amDone(result);
 }
 
@@ -45,7 +53,7 @@ Goal::Co PathSubstitutionGoal::init()
 
     /* If the path already exists we're done. */
     if (!repair && worker.store.isValidPath(storePath)) {
-        co_return done(ecSuccess, BuildResult::AlreadyValid);
+        co_return doneSuccess(BuildResult::Success::AlreadyValid);
     }
 
     if (settings.readOnlyMode)
@@ -165,9 +173,9 @@ Goal::Co PathSubstitutionGoal::init()
     /* Hack: don't indicate failure if there were no substituters.
        In that case the calling derivation should just do a
        build. */
-    co_return done(
+    co_return doneFailure(
         substituterFailed ? ecFailed : ecNoSubstituters,
-        BuildResult::NoSubstituters,
+        BuildResult::Failure::NoSubstituters,
         fmt("path '%s' is required, but there is no substituter that can build it",
             worker.store.printStorePath(storePath)));
 }
@@ -178,9 +186,9 @@ Goal::Co PathSubstitutionGoal::tryToRun(
     trace("all references realised");
 
     if (nrFailed > 0) {
-        co_return done(
+        co_return doneFailure(
             nrNoSubstituters > 0 ? ecNoSubstituters : ecFailed,
-            BuildResult::DependencyFailed,
+            BuildResult::Failure::DependencyFailed,
             fmt("some references of path '%s' could not be realised", worker.store.printStorePath(storePath)));
     }
 
@@ -297,7 +305,7 @@ Goal::Co PathSubstitutionGoal::tryToRun(
 
     worker.updateProgress();
 
-    co_return done(ecSuccess, BuildResult::Substituted);
+    co_return doneSuccess(BuildResult::Success::Substituted);
 }
 
 void PathSubstitutionGoal::handleEOF(Descriptor fd)

src/libstore/derivation-options.cc

@@ -266,7 +266,9 @@ DerivationOptions::getParsedExportReferencesGraph(const StoreDirConfig & store)
         for (auto & storePathS : ss) {
             if (!store.isInStore(storePathS))
                 throw BuildError(
-                    BuildResult::InputRejected, "'exportReferencesGraph' contains a non-store path '%1%'", storePathS);
+                    BuildResult::Failure::InputRejected,
+                    "'exportReferencesGraph' contains a non-store path '%1%'",
+                    storePathS);
             storePaths.insert(store.toStorePath(storePathS).first);
         }
         res.insert_or_assign(fileName, storePaths);

src/libstore/include/nix/store/build-result.hh

@@ -12,34 +12,67 @@ namespace nix {
 
 struct BuildResult
 {
+    struct Success
+    {
         /**
          * @note This is directly used in the nix-store --serve protocol.
          * That means we need to worry about compatibility across versions.
          * Therefore, don't remove status codes, and only add new status
          * codes at the end of the list.
+         *
+         * Must be disjoint with `Failure::Status`.
          */
-    enum Status {
+        enum Status : uint8_t {
             Built = 0,
-        Substituted,
+            Substituted = 1,
-        AlreadyValid,
+            AlreadyValid = 2,
-        PermanentFailure,
+            ResolvesToAlreadyValid = 13,
-        InputRejected,
+        } status;
-        OutputRejected,
+
+        /**
+         * For derivations, a mapping from the names of the wanted outputs
+         * to actual paths.
+         */
+        SingleDrvOutputs builtOutputs;
+
+        bool operator==(const BuildResult::Success &) const noexcept;
+        std::strong_ordering operator<=>(const BuildResult::Success &) const noexcept;
+
+        static bool statusIs(uint8_t status)
+        {
+            return status == Built || status == Substituted || status == AlreadyValid
+                   || status == ResolvesToAlreadyValid;
+        }
+    };
+
+    struct Failure
+    {
+        /**
+         * @note This is directly used in the nix-store --serve protocol.
+         * That means we need to worry about compatibility across versions.
+         * Therefore, don't remove status codes, and only add new status
+         * codes at the end of the list.
+         *
+         * Must be disjoint with `Success::Status`.
+         */
+        enum Status : uint8_t {
+            PermanentFailure = 3,
+            InputRejected = 4,
+            OutputRejected = 5,
             /// possibly transient
-        TransientFailure,
+            TransientFailure = 6,
             /// no longer used
-        CachedFailure,
+            CachedFailure = 7,
-        TimedOut,
+            TimedOut = 8,
-        MiscFailure,
+            MiscFailure = 9,
-        DependencyFailed,
+            DependencyFailed = 10,
-        LogLimitExceeded,
+            LogLimitExceeded = 11,
-        NotDeterministic,
+            NotDeterministic = 12,
-        ResolvesToAlreadyValid,
+            NoSubstituters = 14,
-        NoSubstituters,
             /// A certain type of `OutputRejected`. The protocols do not yet
             /// know about this one, so change it back to `OutputRejected`
             /// before serialization.
-        HashMismatch,
+            HashMismatch = 15,
         } status = MiscFailure;
 
         /**
@@ -50,11 +83,6 @@ struct BuildResult
          */
         std::string errorMsg;
 
-    /**
-     * How many times this build was performed.
-     */
-    unsigned int timesBuilt = 0;
-
         /**
          * If timesBuilt > 1, whether some builds did not produce the same
          * result. (Note that 'isNonDeterministic = false' does not mean
@@ -63,11 +91,41 @@ struct BuildResult
          */
         bool isNonDeterministic = false;
 
+        bool operator==(const BuildResult::Failure &) const noexcept;
+        std::strong_ordering operator<=>(const BuildResult::Failure &) const noexcept;
+
+        [[noreturn]] void rethrow() const
+        {
+            throw Error("%s", errorMsg);
+        }
+    };
+
+    std::variant<Success, Failure> inner = Failure{};
+
     /**
-     * For derivations, a mapping from the names of the wanted outputs
+     * Convenience wrapper to avoid a longer `std::get_if` usage by the
-     * to actual paths.
+     * caller (which will have to add more `BuildResult::` than we do
+     * below also, do note.)
      */
-    SingleDrvOutputs builtOutputs;
+    auto * tryGetSuccess(this auto & self)
+    {
+        return std::get_if<Success>(&self.inner);
+    }
+
+    /**
+     * Convenience wrapper to avoid a longer `std::get_if` usage by the
+     * caller (which will have to add more `BuildResult::` than we do
+     * below also, do note.)
+     */
+    auto * tryGetFailure(this auto & self)
+    {
+        return std::get_if<Failure>(&self.inner);
+    }
+
+    /**
+     * How many times this build was performed.
+     */
+    unsigned int timesBuilt = 0;
 
     /**
      * The start/stop times of the build (or one of the rounds, if it
@@ -82,16 +140,6 @@ struct BuildResult
 
     bool operator==(const BuildResult &) const noexcept;
     std::strong_ordering operator<=>(const BuildResult &) const noexcept;
-
-    bool success()
-    {
-        return status == Built || status == Substituted || status == AlreadyValid || status == ResolvesToAlreadyValid;
-    }
-
-    void rethrow()
-    {
-        throw Error("%s", errorMsg);
-    }
 };
 
 /**
@@ -99,15 +147,9 @@ struct BuildResult
  */
 struct BuildError : public Error
 {
-    BuildResult::Status status;
+    BuildResult::Failure::Status status;
 
-    BuildError(BuildResult::Status status, BuildError && error)
+    BuildError(BuildResult::Failure::Status status, auto &&... args)
-        : Error{std::move(error)}
-        , status{status}
-    {
-    }
-
-    BuildError(BuildResult::Status status, auto &&... args)
         : Error{args...}
         , status{status}
     {

src/libstore/include/nix/store/build/derivation-builder.hh

@@ -22,7 +22,7 @@ struct BuilderFailureError : BuildError
 
     std::string extraMsgAfter;
 
-    BuilderFailureError(BuildResult::Status status, int builderStatus, std::string extraMsgAfter)
+    BuilderFailureError(BuildResult::Failure::Status status, int builderStatus, std::string extraMsgAfter)
         : BuildError{
             status,
               /* No message for now, because the caller will make for

src/libstore/include/nix/store/build/derivation-building-goal.hh

@@ -147,7 +147,7 @@ private:
      */
     void killChild();
 
-    Done doneSuccess(BuildResult::Status status, SingleDrvOutputs builtOutputs);
+    Done doneSuccess(BuildResult::Success::Status status, SingleDrvOutputs builtOutputs);
 
     Done doneFailure(BuildError ex);
 

src/libstore/include/nix/store/build/derivation-goal.hh

@@ -99,7 +99,7 @@ private:
 
     Co repairClosure();
 
-    Done doneSuccess(BuildResult::Status status, Realisation builtOutput);
+    Done doneSuccess(BuildResult::Success::Status status, Realisation builtOutput);
 
     Done doneFailure(BuildError ex);
 };

src/libstore/include/nix/store/build/substitution-goal.hh

@@ -41,7 +41,9 @@ struct PathSubstitutionGoal : public Goal
      */
     std::optional<ContentAddress> ca;
 
-    Done done(ExitCode result, BuildResult::Status status, std::optional<std::string> errorMsg = {});
+    Done doneSuccess(BuildResult::Success::Status status);
+
+    Done doneFailure(ExitCode result, BuildResult::Failure::Status status, std::string errorMsg);
 
 public:
     PathSubstitutionGoal(

src/libstore/legacy-ssh-store.cc

@@ -241,12 +241,13 @@ void LegacySSHStore::buildPaths(
 
     conn->to.flush();
 
-    BuildResult result;
+    auto status = readInt(conn->from);
-    result.status = (BuildResult::Status) readInt(conn->from);
+    if (!BuildResult::Success::statusIs(status)) {
-
+        BuildResult::Failure failure{
-    if (!result.success()) {
+            .status = (BuildResult::Failure::Status) status,
-        conn->from >> result.errorMsg;
+        };
-        throw Error(result.status, result.errorMsg);
+        conn->from >> failure.errorMsg;
+        throw Error(failure.status, std::move(failure.errorMsg));
     }
 }
 

src/libstore/local-store.cc

@@ -997,7 +997,7 @@ void LocalStore::registerValidPaths(const ValidPathInfos & infos)
             }},
             {[&](const StorePath & path, const StorePath & parent) {
                 return BuildError(
-                    BuildResult::OutputRejected,
+                    BuildResult::Failure::OutputRejected,
                     "cycle detected in the references of '%s' from '%s'",
                     printStorePath(path),
                     printStorePath(parent));

src/libstore/misc.cc

@@ -322,7 +322,7 @@ StorePaths Store::topoSortPaths(const StorePathSet & paths)
         }},
         {[&](const StorePath & path, const StorePath & parent) {
             return BuildError(
-                BuildResult::OutputRejected,
+                BuildResult::Failure::OutputRejected,
                 "cycle detected in the references of '%s' from '%s'",
                 printStorePath(path),
                 printStorePath(parent));

src/libstore/posix-fs-canonicalise.cc

@@ -98,7 +98,7 @@ static void canonicalisePathMetaData_(
        (i.e. "touch $out/foo; ln $out/foo $out/bar"). */
     if (uidRange && (st.st_uid < uidRange->first || st.st_uid > uidRange->second)) {
         if (S_ISDIR(st.st_mode) || !inodesSeen.count(Inode(st.st_dev, st.st_ino)))
-            throw BuildError(BuildResult::OutputRejected, "invalid ownership on file '%1%'", path);
+            throw BuildError(BuildResult::Failure::OutputRejected, "invalid ownership on file '%1%'", path);
         mode_t mode = st.st_mode & ~S_IFMT;
         assert(
             S_ISLNK(st.st_mode)

src/libstore/remote-store.cc

@@ -598,16 +598,15 @@ std::vector<KeyedBuildResult> RemoteStore::buildPathsWithResults(
                     [&](const DerivedPath::Opaque & bo) {
                         results.push_back(
                             KeyedBuildResult{
-                                {
+                                {.inner{BuildResult::Success{
-                                    .status = BuildResult::Substituted,
+                                    .status = BuildResult::Success::Substituted,
-                                },
+                                }}},
                                 /* .path = */ bo,
                             });
                     },
                     [&](const DerivedPath::Built & bfd) {
-                        KeyedBuildResult res{
+                        BuildResult::Success success{
-                            {.status = BuildResult::Built},
+                            .status = BuildResult::Success::Built,
-                            /* .path = */ bfd,
                         };
 
                         OutputPathMap outputs;
@@ -627,9 +626,9 @@ std::vector<KeyedBuildResult> RemoteStore::buildPathsWithResults(
                                 auto realisation = queryRealisation(outputId);
                                 if (!realisation)
                                     throw MissingRealisation(outputId);
-                                res.builtOutputs.emplace(output, *realisation);
+                                success.builtOutputs.emplace(output, *realisation);
                             } else {
-                                res.builtOutputs.emplace(
+                                success.builtOutputs.emplace(
                                     output,
                                     Realisation{
                                         .id = outputId,
@@ -638,7 +637,11 @@ std::vector<KeyedBuildResult> RemoteStore::buildPathsWithResults(
                             }
                         }
 
-                        results.push_back(res);
+                        results.push_back(
+                            KeyedBuildResult{
+                                {.inner = std::move(success)},
+                                /* .path = */ bfd,
+                            });
                     }},
                 path.raw());
         }

src/libstore/restricted-store.cc

@@ -257,8 +257,8 @@ void RestrictedStore::buildPaths(
     const std::vector<DerivedPath> & paths, BuildMode buildMode, std::shared_ptr<Store> evalStore)
 {
     for (auto & result : buildPathsWithResults(paths, buildMode, evalStore))
-        if (!result.success())
+        if (auto * failureP = result.tryGetFailure())
-            result.rethrow();
+            failureP->rethrow();
 }
 
 std::vector<KeyedBuildResult> RestrictedStore::buildPathsWithResults(
@@ -280,11 +280,13 @@ std::vector<KeyedBuildResult> RestrictedStore::buildPathsWithResults(
     auto results = next->buildPathsWithResults(paths, buildMode);
 
     for (auto & result : results) {
-        for (auto & [outputName, output] : result.builtOutputs) {
+        if (auto * successP = result.tryGetSuccess()) {
+            for (auto & [outputName, output] : successP->builtOutputs) {
                 newPaths.insert(output.outPath);
                 newRealisations.insert(output);
             }
         }
+    }
 
     StorePathSet closure;
     next->computeFSClosure(newPaths, closure);

src/libstore/serve-protocol.cc

@@ -16,32 +16,62 @@ namespace nix {
 BuildResult ServeProto::Serialise<BuildResult>::read(const StoreDirConfig & store, ServeProto::ReadConn conn)
 {
     BuildResult status;
-    status.status = (BuildResult::Status) readInt(conn.from);
+    BuildResult::Success success;
-    conn.from >> status.errorMsg;
+    BuildResult::Failure failure;
+
+    auto rawStatus = readInt(conn.from);
+    conn.from >> failure.errorMsg;
 
     if (GET_PROTOCOL_MINOR(conn.version) >= 3)
-        conn.from >> status.timesBuilt >> status.isNonDeterministic >> status.startTime >> status.stopTime;
+        conn.from >> status.timesBuilt >> failure.isNonDeterministic >> status.startTime >> status.stopTime;
     if (GET_PROTOCOL_MINOR(conn.version) >= 6) {
         auto builtOutputs = ServeProto::Serialise<DrvOutputs>::read(store, conn);
         for (auto && [output, realisation] : builtOutputs)
-            status.builtOutputs.insert_or_assign(std::move(output.outputName), std::move(realisation));
+            success.builtOutputs.insert_or_assign(std::move(output.outputName), std::move(realisation));
     }
+
+    if (BuildResult::Success::statusIs(rawStatus)) {
+        success.status = static_cast<BuildResult::Success::Status>(rawStatus);
+        status.inner = std::move(success);
+    } else {
+        failure.status = static_cast<BuildResult::Failure::Status>(rawStatus);
+        status.inner = std::move(failure);
+    }
+
     return status;
 }
 
 void ServeProto::Serialise<BuildResult>::write(
-    const StoreDirConfig & store, ServeProto::WriteConn conn, const BuildResult & status)
+    const StoreDirConfig & store, ServeProto::WriteConn conn, const BuildResult & res)
 {
-    conn.to << status.status << status.errorMsg;
+    /* The protocol predates the use of sum types (std::variant) to
-
+       separate the success or failure cases. As such, it transits some
+       success- or failure-only fields in both cases. This helper
+       function helps support this: in each case, we just pass the old
+       default value for the fields that don't exist in that case. */
+    auto common = [&](std::string_view errorMsg, bool isNonDeterministic, const auto & builtOutputs) {
+        conn.to << errorMsg;
         if (GET_PROTOCOL_MINOR(conn.version) >= 3)
-        conn.to << status.timesBuilt << status.isNonDeterministic << status.startTime << status.stopTime;
+            conn.to << res.timesBuilt << isNonDeterministic << res.startTime << res.stopTime;
         if (GET_PROTOCOL_MINOR(conn.version) >= 6) {
-        DrvOutputs builtOutputs;
+            DrvOutputs builtOutputsFullKey;
-        for (auto & [output, realisation] : status.builtOutputs)
+            for (auto & [output, realisation] : builtOutputs)
-            builtOutputs.insert_or_assign(realisation.id, realisation);
+                builtOutputsFullKey.insert_or_assign(realisation.id, realisation);
-        ServeProto::write(store, conn, builtOutputs);
+            ServeProto::write(store, conn, builtOutputsFullKey);
         }
+    };
+    std::visit(
+        overloaded{
+            [&](const BuildResult::Failure & failure) {
+                conn.to << failure.status;
+                common(failure.errorMsg, failure.isNonDeterministic, decltype(BuildResult::Success::builtOutputs){});
+            },
+            [&](const BuildResult::Success & success) {
+                conn.to << success.status;
+                common(/*errorMsg=*/"", /*isNonDeterministic=*/false, success.builtOutputs);
+            },
+        },
+        res.inner);
 }
 
 UnkeyedValidPathInfo ServeProto::Serialise<UnkeyedValidPathInfo>::read(const StoreDirConfig & store, ReadConn conn)

src/libstore/store-api.cc

@@ -774,7 +774,7 @@ StorePathSet Store::exportReferences(const StorePathSet & storePaths, const Stor
     for (auto & storePath : storePaths) {
         if (!inputPaths.count(storePath))
             throw BuildError(
-                BuildResult::InputRejected,
+                BuildResult::Failure::InputRejected,
                 "cannot export references of path '%s' because it is not in the input closure of the derivation",
                 printStorePath(storePath));
 

src/libstore/unix/build/derivation-builder.cc

@@ -51,7 +51,7 @@ namespace nix {
 struct NotDeterministic : BuildError
 {
     NotDeterministic(auto &&... args)
-        : BuildError(BuildResult::NotDeterministic, args...)
+        : BuildError(BuildResult::Failure::NotDeterministic, args...)
     {
     }
 };
@@ -519,7 +519,8 @@ SingleDrvOutputs DerivationBuilderImpl::unprepareBuild()
         cleanupBuild(false);
 
         throw BuilderFailureError{
-            !derivationType.isSandboxed() || diskFull ? BuildResult::TransientFailure : BuildResult::PermanentFailure,
+            !derivationType.isSandboxed() || diskFull ? BuildResult::Failure::TransientFailure
+                                                      : BuildResult::Failure::PermanentFailure,
             status,
             diskFull ? "\nnote: build failure may have been caused by lack of free disk space" : "",
         };
@@ -701,7 +702,7 @@ std::optional<Descriptor> DerivationBuilderImpl::startBuild()
                 fmt("\nNote: run `%s` to run programs for x86_64-darwin",
                     Magenta("/usr/sbin/softwareupdate --install-rosetta && launchctl stop org.nixos.nix-daemon"));
 
-        throw BuildError(BuildResult::InputRejected, msg);
+        throw BuildError(BuildResult::Failure::InputRejected, msg);
     }
 
     auto buildDir = store.config->getBuildDir();
@@ -1389,7 +1390,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
         auto optSt = maybeLstat(actualPath.c_str());
         if (!optSt)
             throw BuildError(
-                BuildResult::OutputRejected,
+                BuildResult::Failure::OutputRejected,
                 "builder for '%s' failed to produce output path for output '%s' at '%s'",
                 store.printStorePath(drvPath),
                 outputName,
@@ -1404,7 +1405,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
         if ((!S_ISLNK(st.st_mode) && (st.st_mode & (S_IWGRP | S_IWOTH)))
             || (buildUser && st.st_uid != buildUser->getUID()))
             throw BuildError(
-                BuildResult::OutputRejected,
+                BuildResult::Failure::OutputRejected,
                 "suspicious ownership or permission on '%s' for output '%s'; rejecting this build output",
                 actualPath,
                 outputName);
@@ -1442,7 +1443,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
             auto orifu = get(outputReferencesIfUnregistered, name);
             if (!orifu)
                 throw BuildError(
-                    BuildResult::OutputRejected,
+                    BuildResult::Failure::OutputRejected,
                     "no output reference for '%s' in build of '%s'",
                     name,
                     store.printStorePath(drvPath));
@@ -1467,7 +1468,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
         {[&](const std::string & path, const std::string & parent) {
             // TODO with more -vvvv also show the temporary paths for manual inspection.
             return BuildError(
-                BuildResult::OutputRejected,
+                BuildResult::Failure::OutputRejected,
                 "cycle detected in build of '%s' in the references of output '%s' from output '%s'",
                 store.printStorePath(drvPath),
                 path,
@@ -1561,12 +1562,13 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
         auto newInfoFromCA = [&](const DerivationOutput::CAFloating outputHash) -> ValidPathInfo {
             auto st = get(outputStats, outputName);
             if (!st)
-                throw BuildError(BuildResult::OutputRejected, "output path %1% without valid stats info", actualPath);
+                throw BuildError(
+                    BuildResult::Failure::OutputRejected, "output path %1% without valid stats info", actualPath);
             if (outputHash.method.getFileIngestionMethod() == FileIngestionMethod::Flat) {
                 /* The output path should be a regular file without execute permission. */
                 if (!S_ISREG(st->st_mode) || (st->st_mode & S_IXUSR) != 0)
                     throw BuildError(
-                        BuildResult::OutputRejected,
+                        BuildResult::Failure::OutputRejected,
                         "output path '%1%' should be a non-executable regular file "
                         "since recursive hashing is not enabled (one of outputHashMode={flat,text} is true)",
                         actualPath);

src/libstore/worker-protocol.cc

@@ -165,10 +165,14 @@ void WorkerProto::Serialise<KeyedBuildResult>::write(
 BuildResult WorkerProto::Serialise<BuildResult>::read(const StoreDirConfig & store, WorkerProto::ReadConn conn)
 {
     BuildResult res;
-    res.status = static_cast<BuildResult::Status>(readInt(conn.from));
+    BuildResult::Success success;
-    conn.from >> res.errorMsg;
+    BuildResult::Failure failure;
+
+    auto rawStatus = readInt(conn.from);
+    conn.from >> failure.errorMsg;
+
     if (GET_PROTOCOL_MINOR(conn.version) >= 29) {
-        conn.from >> res.timesBuilt >> res.isNonDeterministic >> res.startTime >> res.stopTime;
+        conn.from >> res.timesBuilt >> failure.isNonDeterministic >> res.startTime >> res.stopTime;
     }
     if (GET_PROTOCOL_MINOR(conn.version) >= 37) {
         res.cpuUser = WorkerProto::Serialise<std::optional<std::chrono::microseconds>>::read(store, conn);
@@ -177,28 +181,56 @@ BuildResult WorkerProto::Serialise<BuildResult>::read(const StoreDirConfig & sto
     if (GET_PROTOCOL_MINOR(conn.version) >= 28) {
         auto builtOutputs = WorkerProto::Serialise<DrvOutputs>::read(store, conn);
         for (auto && [output, realisation] : builtOutputs)
-            res.builtOutputs.insert_or_assign(std::move(output.outputName), std::move(realisation));
+            success.builtOutputs.insert_or_assign(std::move(output.outputName), std::move(realisation));
     }
+
+    if (BuildResult::Success::statusIs(rawStatus)) {
+        success.status = static_cast<BuildResult::Success::Status>(rawStatus);
+        res.inner = std::move(success);
+    } else {
+        failure.status = static_cast<BuildResult::Failure::Status>(rawStatus);
+        res.inner = std::move(failure);
+    }
+
     return res;
 }
 
 void WorkerProto::Serialise<BuildResult>::write(
     const StoreDirConfig & store, WorkerProto::WriteConn conn, const BuildResult & res)
 {
-    conn.to << res.status << res.errorMsg;
+    /* The protocol predates the use of sum types (std::variant) to
+       separate the success or failure cases. As such, it transits some
+       success- or failure-only fields in both cases. This helper
+       function helps support this: in each case, we just pass the old
+       default value for the fields that don't exist in that case. */
+    auto common = [&](std::string_view errorMsg, bool isNonDeterministic, const auto & builtOutputs) {
+        conn.to << errorMsg;
         if (GET_PROTOCOL_MINOR(conn.version) >= 29) {
-        conn.to << res.timesBuilt << res.isNonDeterministic << res.startTime << res.stopTime;
+            conn.to << res.timesBuilt << isNonDeterministic << res.startTime << res.stopTime;
         }
         if (GET_PROTOCOL_MINOR(conn.version) >= 37) {
             WorkerProto::write(store, conn, res.cpuUser);
             WorkerProto::write(store, conn, res.cpuSystem);
         }
         if (GET_PROTOCOL_MINOR(conn.version) >= 28) {
-        DrvOutputs builtOutputs;
+            DrvOutputs builtOutputsFullKey;
-        for (auto & [output, realisation] : res.builtOutputs)
+            for (auto & [output, realisation] : builtOutputs)
-            builtOutputs.insert_or_assign(realisation.id, realisation);
+                builtOutputsFullKey.insert_or_assign(realisation.id, realisation);
-        WorkerProto::write(store, conn, builtOutputs);
+            WorkerProto::write(store, conn, builtOutputsFullKey);
         }
+    };
+    std::visit(
+        overloaded{
+            [&](const BuildResult::Failure & failure) {
+                conn.to << failure.status;
+                common(failure.errorMsg, failure.isNonDeterministic, decltype(BuildResult::Success::builtOutputs){});
+            },
+            [&](const BuildResult::Success & success) {
+                conn.to << success.status;
+                common(/*errorMsg=*/"", /*isNonDeterministic=*/false, success.builtOutputs);
+            },
+        },
+        res.inner);
 }
 
 ValidPathInfo WorkerProto::Serialise<ValidPathInfo>::read(const StoreDirConfig & store, ReadConn conn)

src/nix/build-remote/build-remote.cc

@@ -324,7 +324,7 @@ static int main_build_remote(int argc, char ** argv)
                 drv.inputSrcs = store->parseStorePathSet(inputs);
             optResult = sshStore->buildDerivation(*drvPath, (const BasicDerivation &) drv);
             auto & result = *optResult;
-            if (!result.success()) {
+            if (auto * failureP = result.tryGetFailure()) {
                 if (settings.keepFailed) {
                     warn(
                         "The failed build directory was kept on the remote builder due to `--keep-failed`.%s",
@@ -333,7 +333,7 @@ static int main_build_remote(int argc, char ** argv)
                             : "");
                 }
                 throw Error(
-                    "build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri, result.errorMsg);
+                    "build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri, failureP->errorMsg);
             }
         } else {
             copyClosure(*store, *sshStore, StorePathSet{*drvPath}, NoRepair, NoCheckSigs, substitute);
@@ -357,13 +357,16 @@ static int main_build_remote(int argc, char ** argv)
                     debug("missing output %s", outputName);
                     assert(optResult);
                     auto & result = *optResult;
-                    auto i = result.builtOutputs.find(outputName);
+                    if (auto * successP = result.tryGetSuccess()) {
-                    assert(i != result.builtOutputs.end());
+                        auto & success = *successP;
+                        auto i = success.builtOutputs.find(outputName);
+                        assert(i != success.builtOutputs.end());
                         auto & newRealisation = i->second;
                         missingRealisations.insert(newRealisation);
                         missingPaths.insert(newRealisation.outPath);
                     }
                 }
+            }
         } else {
             auto outputPaths = drv.outputsAndOptPaths(*store);
             for (auto & [outputName, hopefullyOutputPath] : outputPaths) {

tests/functional/test-libstoreconsumer/main.cc

@@ -34,10 +34,12 @@ int main(int argc, char ** argv)
         const auto results = store->buildPathsWithResults(paths, bmNormal, store);
 
         for (const auto & result : results) {
-            for (const auto & [outputName, realisation] : result.builtOutputs) {
+            if (auto * successP = result.tryGetSuccess()) {
+                for (const auto & [outputName, realisation] : successP->builtOutputs) {
                     std::cout << store->printStorePath(realisation.outPath) << "\n";
                 }
             }
+        }
 
         return 0;