Create proper serializer for BuildResult status

Ericson2314 · Ericson2314 · commit 06fc951617fc · 2025-12-08T23:31:09.000-05:00
The casts were not safe with respect to unknown values, but these are.
diff --git a/src/libstore/common-protocol.cc b/src/libstore/common-protocol.cc
@@ -99,4 +99,51 @@ void CommonProto::Serialise<std::optional<ContentAddress>>::write(
     conn.to << (caOpt ? renderContentAddress(*caOpt) : "");
 }
 
+/**
+ * Mapping from protocol wire values to BuildResultStatus.
+ *
+ * The array index is the wire value.
+ * Note: HashMismatch is not in the protocol; it gets converted
+ * to OutputRejected before serialization.
+ */
+constexpr static BuildResultStatus buildResultStatusTable[] = {
+    BuildResultSuccessStatus::Built,                  // 0
+    BuildResultSuccessStatus::Substituted,            // 1
+    BuildResultSuccessStatus::AlreadyValid,           // 2
+    BuildResultFailureStatus::PermanentFailure,       // 3
+    BuildResultFailureStatus::InputRejected,          // 4
+    BuildResultFailureStatus::OutputRejected,         // 5
+    BuildResultFailureStatus::TransientFailure,       // 6
+    BuildResultFailureStatus::CachedFailure,          // 7
+    BuildResultFailureStatus::TimedOut,               // 8
+    BuildResultFailureStatus::MiscFailure,            // 9
+    BuildResultFailureStatus::DependencyFailed,       // 10
+    BuildResultFailureStatus::LogLimitExceeded,       // 11
+    BuildResultFailureStatus::NotDeterministic,       // 12
+    BuildResultSuccessStatus::ResolvesToAlreadyValid, // 13
+    BuildResultFailureStatus::NoSubstituters,         // 14
+};
+
+BuildResultStatus
+CommonProto::Serialise<BuildResultStatus>::read(const StoreDirConfig & store, CommonProto::ReadConn conn)
+{
+    auto rawStatus = readNum<uint8_t>(conn.from);
+
+    if (rawStatus >= std::size(buildResultStatusTable))
+        throw Error("Invalid BuildResult status %d from remote", rawStatus);
+
+    return buildResultStatusTable[rawStatus];
+}
+
+void CommonProto::Serialise<BuildResultStatus>::write(
+    const StoreDirConfig & store, CommonProto::WriteConn conn, const BuildResultStatus & status)
+{
+    for (auto && [wire, val] : enumerate(buildResultStatusTable))
+        if (val == status) {
+            conn.to << uint8_t(wire);
+            return;
+        }
+    unreachable();
+}
+
 } // namespace nix
diff --git a/src/libstore/include/nix/store/build-result.hh b/src/libstore/include/nix/store/build-result.hh
@@ -11,24 +11,52 @@
 
 namespace nix {
 
+/**
+ * Names must be disjoint with `BuildResultFailureStatus`.
+ *
+ * @note Prefer using `BuildResult::Success::Status`, this name is just
+ * for sake of forward declarations.
+ */
+enum struct BuildResultSuccessStatus : uint8_t {
+    Built,
+    Substituted,
+    AlreadyValid,
+    ResolvesToAlreadyValid,
+};
+
+/**
+ * Names must be disjoint with `BuildResultSuccessStatus`.
+ *
+ * @note Prefer using `BuildResult::Failure::Status`, this name is just
+ * for sake of forward declarations.
+ */
+enum struct BuildResultFailureStatus : uint8_t {
+    PermanentFailure,
+    InputRejected,
+    OutputRejected,
+    /// possibly transient
+    TransientFailure,
+    /// no longer used
+    CachedFailure,
+    TimedOut,
+    MiscFailure,
+    DependencyFailed,
+    LogLimitExceeded,
+    NotDeterministic,
+    NoSubstituters,
+    /// A certain type of `OutputRejected`. The protocols do not yet
+    /// know about this one, so change it back to `OutputRejected`
+    /// before serialization.
+    HashMismatch,
+};
+
 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 : uint8_t {
-            Built = 0,
-            Substituted = 1,
-            AlreadyValid = 2,
-            ResolvesToAlreadyValid = 13,
-        } status;
+        using Status = enum BuildResultSuccessStatus;
+        using enum Status;
+        Status status;
 
         /**
          * For derivations, a mapping from the names of the wanted outputs
@@ -38,43 +66,13 @@ struct BuildResult
 
         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 = 6,
-            /// no longer used
-            CachedFailure = 7,
-            TimedOut = 8,
-            MiscFailure = 9,
-            DependencyFailed = 10,
-            LogLimitExceeded = 11,
-            NotDeterministic = 12,
-            NoSubstituters = 14,
-            /// A certain type of `OutputRejected`. The protocols do not yet
-            /// know about this one, so change it back to `OutputRejected`
-            /// before serialization.
-            HashMismatch = 15,
-        } status = MiscFailure;
+        using Status = enum BuildResultFailureStatus;
+        using enum Status;
+        Status status = MiscFailure;
 
         /**
          * Information about the error if the build failed.
diff --git a/src/libstore/include/nix/store/common-protocol.hh b/src/libstore/include/nix/store/common-protocol.hh
@@ -3,6 +3,8 @@
 
 #include "nix/util/serialise.hh"
 
+#include <variant>
+
 namespace nix {
 
 struct StoreDirConfig;
@@ -13,6 +15,8 @@ class StorePath;
 struct ContentAddress;
 struct DrvOutput;
 struct Realisation;
+enum struct BuildResultSuccessStatus : uint8_t;
+enum struct BuildResultFailureStatus : uint8_t;
 
 /**
  * Shared serializers between the worker protocol, serve protocol, and a
@@ -83,7 +87,6 @@ DECLARE_COMMON_SERIALISER(std::tuple<Ts...>);
 
 template<typename K, typename V>
 DECLARE_COMMON_SERIALISER(std::map<K COMMA_ V>);
-#undef COMMA_
 
 /**
  * These use the empty string for the null case, relying on the fact
@@ -104,4 +107,14 @@ DECLARE_COMMON_SERIALISER(std::optional<StorePath>);
 template<>
 DECLARE_COMMON_SERIALISER(std::optional<ContentAddress>);
 
+/**
+ * The success and failure codes never overlay in enum tag values in the wire formats
+ */
+using BuildResultStatus = std::variant<BuildResultSuccessStatus, BuildResultFailureStatus>;
+
+template<>
+DECLARE_COMMON_SERIALISER(BuildResultStatus);
+
+#undef COMMA_
+
 } // namespace nix
diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc
@@ -3,6 +3,7 @@
 #include "nix/util/archive.hh"
 #include "nix/util/pool.hh"
 #include "nix/store/remote-store.hh"
+#include "nix/store/common-protocol.hh"
 #include "nix/store/serve-protocol.hh"
 #include "nix/store/serve-protocol-connection.hh"
 #include "nix/store/serve-protocol-impl.hh"
@@ -241,13 +242,11 @@ void LegacySSHStore::buildPaths(
 
     conn->to.flush();
 
-    auto status = readInt(conn->from);
-    if (!BuildResult::Success::statusIs(status)) {
-        BuildResult::Failure failure{
-            .status = (BuildResult::Failure::Status) status,
-        };
-        conn->from >> failure.errorMsg;
-        throw Error(failure.status, std::move(failure.errorMsg));
+    auto status = CommonProto::Serialise<BuildResultStatus>::read(*this, {conn->from});
+    if (auto * failure = std::get_if<BuildResultFailureStatus>(&status)) {
+        std::string errorMsg;
+        conn->from >> errorMsg;
+        throw BuildError(*failure, std::move(errorMsg));
     }
 }
 
diff --git a/src/libstore/serve-protocol.cc b/src/libstore/serve-protocol.cc
@@ -2,6 +2,7 @@
 #include "nix/store/path-with-outputs.hh"
 #include "nix/store/store-api.hh"
 #include "nix/store/build-result.hh"
+#include "nix/store/common-protocol.hh"
 #include "nix/store/serve-protocol.hh"
 #include "nix/store/serve-protocol-impl.hh"
 #include "nix/util/archive.hh"
@@ -15,30 +16,35 @@ namespace nix {
 
 BuildResult ServeProto::Serialise<BuildResult>::read(const StoreDirConfig & store, ServeProto::ReadConn conn)
 {
-    BuildResult status;
+    BuildResult res;
     BuildResult::Success success;
     BuildResult::Failure failure;
 
-    auto rawStatus = readInt(conn.from);
+    auto status = ServeProto::Serialise<BuildResultStatus>::read(store, {conn.from});
     conn.from >> failure.errorMsg;
 
     if (GET_PROTOCOL_MINOR(conn.version) >= 3)
-        conn.from >> status.timesBuilt >> failure.isNonDeterministic >> status.startTime >> status.stopTime;
+        conn.from >> res.timesBuilt >> failure.isNonDeterministic >> res.startTime >> res.stopTime;
     if (GET_PROTOCOL_MINOR(conn.version) >= 6) {
         auto builtOutputs = ServeProto::Serialise<DrvOutputs>::read(store, conn);
         for (auto && [output, realisation] : builtOutputs)
             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);
-    }
+    res.inner = std::visit(
+        overloaded{
+            [&](BuildResult::Success::Status s) -> decltype(res.inner) {
+                success.status = s;
+                return std::move(success);
+            },
+            [&](BuildResult::Failure::Status s) -> decltype(res.inner) {
+                failure.status = s;
+                return std::move(failure);
+            },
+        },
+        status);
 
-    return status;
+    return res;
 }
 
 void ServeProto::Serialise<BuildResult>::write(
@@ -63,11 +69,11 @@ void ServeProto::Serialise<BuildResult>::write(
     std::visit(
         overloaded{
             [&](const BuildResult::Failure & failure) {
-                conn.to << failure.status;
+                ServeProto::write(store, {conn.to}, BuildResultStatus{failure.status});
                 common(failure.errorMsg, failure.isNonDeterministic, decltype(BuildResult::Success::builtOutputs){});
             },
             [&](const BuildResult::Success & success) {
-                conn.to << success.status;
+                ServeProto::write(store, {conn.to}, BuildResultStatus{success.status});
                 common(/*errorMsg=*/"", /*isNonDeterministic=*/false, success.builtOutputs);
             },
         },
diff --git a/src/libstore/worker-protocol.cc b/src/libstore/worker-protocol.cc
@@ -3,6 +3,7 @@
 #include "nix/store/store-api.hh"
 #include "nix/store/gc-store.hh"
 #include "nix/store/build-result.hh"
+#include "nix/store/common-protocol.hh"
 #include "nix/store/worker-protocol.hh"
 #include "nix/store/worker-protocol-impl.hh"
 #include "nix/util/archive.hh"
@@ -209,7 +210,7 @@ BuildResult WorkerProto::Serialise<BuildResult>::read(const StoreDirConfig & sto
     BuildResult::Success success;
     BuildResult::Failure failure;
 
-    auto rawStatus = readInt(conn.from);
+    auto status = WorkerProto::Serialise<BuildResultStatus>::read(store, {conn.from});
     conn.from >> failure.errorMsg;
 
     if (GET_PROTOCOL_MINOR(conn.version) >= 29) {
@@ -225,13 +226,18 @@ BuildResult WorkerProto::Serialise<BuildResult>::read(const StoreDirConfig & sto
             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);
-    }
+    res.inner = std::visit(
+        overloaded{
+            [&](BuildResult::Success::Status s) -> decltype(res.inner) {
+                success.status = s;
+                return std::move(success);
+            },
+            [&](BuildResult::Failure::Status s) -> decltype(res.inner) {
+                failure.status = s;
+                return std::move(failure);
+            },
+        },
+        status);
 
     return res;
 }
@@ -263,11 +269,11 @@ void WorkerProto::Serialise<BuildResult>::write(
     std::visit(
         overloaded{
             [&](const BuildResult::Failure & failure) {
-                conn.to << failure.status;
+                WorkerProto::write(store, {conn.to}, BuildResultStatus{failure.status});
                 common(failure.errorMsg, failure.isNonDeterministic, decltype(BuildResult::Success::builtOutputs){});
             },
             [&](const BuildResult::Success & success) {
-                conn.to << success.status;
+                WorkerProto::write(store, {conn.to}, BuildResultStatus{success.status});
                 common(/*errorMsg=*/"", /*isNonDeterministic=*/false, success.builtOutputs);
             },
         },
