Split serialization/deserialization logic in LocalAC

...into writing/reading to CAS and writing/reading entries (to remove code duplications in uplinking and obtaining the result).
The read functionality cannot be replaced by a single function since the CAS key of the entry is required during uplinking. Although the write functionality can, it is also separated into two parts to keep the reverse logic clear.

2 files changed, 111 insertions, 52 deletions

diff --git a/src/buildtool/storage/local_ac.hpp b/src/buildtool/storage/local_ac.hpp
index 49f46b9d..cb4aa171 100644
--- a/src/buildtool/storage/local_ac.hpp
+++ b/src/buildtool/storage/local_ac.hpp
@@ -15,16 +15,19 @@
 #ifndef INCLUDED_SRC_BUILDTOOL_STORAGE_LOCAL_AC_HPP
 #define INCLUDED_SRC_BUILDTOOL_STORAGE_LOCAL_AC_HPP
 
-#include <utility>  // std::move
+#include <memory>
+#include <optional>
+#include <string>
 
 #include "gsl/gsl"
-#include "src/buildtool/execution_api/common/execution_common.hpp"
 #include "src/buildtool/file_system/file_storage.hpp"
 #include "src/buildtool/file_system/file_system_manager.hpp"
+#include "src/buildtool/file_system/object_type.hpp"
 #include "src/buildtool/logging/logger.hpp"
 #include "src/buildtool/storage/config.hpp"
 #include "src/buildtool/storage/local_cas.hpp"
 #include "src/buildtool/storage/uplinker.hpp"
+#include "src/utils/cpp/expected.hpp"
 
 // forward declarations
 namespace build::bazel::remote::execution::v2 {
@@ -96,8 +99,35 @@ class LocalAC {
         file_store_;
     Uplinker<kDoGlobalUplink> const& uplinker_;
 
-    [[nodiscard]] auto ReadResult(bazel_re::Digest const& digest) const noexcept
-        -> std::optional<bazel_re::ActionResult>;
+    /// \brief Add an entry to the ActionCache.
+    /// \param action_id The id of the action that produced the result.
+    /// \param cas_key The key pointing at an ActionResult in the LocalCAS.
+    /// \return True if an entry was successfully added to the storage.
+    [[nodiscard]] auto WriteActionKey(
+        bazel_re::Digest const& action_id,
+        bazel_re::Digest const& cas_key) const noexcept -> bool;
+
+    /// \brief Get the key pointing at an ActionResult in the LocalCAS.
+    /// \param action_id The id of the action that produced the result.
+    /// \return The key of an Action pointing at an ActionResult in the LocalCAS
+    /// on success or an error message on failure.
+    [[nodiscard]] auto ReadActionKey(bazel_re::Digest const& action_id)
+        const noexcept -> expected<bazel_re::Digest, std::string>;
+
+    /// \brief Add an action to the LocalCAS.
+    /// \param action The action result to store.
+    /// \return The key pointing at an ActionResult present in the LocalCAS on
+    /// success or std::nullopt on failure.
+    [[nodiscard]] auto WriteAction(bazel_re::ActionResult const& action)
+        const noexcept -> std::optional<bazel_re::Digest>;
+
+    /// \brief Get the action specified by a key from the LocalCAS.
+    /// \param cas_key The key pointing at an ActionResult present in the
+    /// LocalCAS.
+    /// \return The ActionResult corresponding to a cas_key on success
+    /// or std::nullopt on failure.
+    [[nodiscard]] auto ReadAction(bazel_re::Digest const& cas_key)
+        const noexcept -> std::optional<bazel_re::ActionResult>;
 };
 
 #ifndef BOOTSTRAP_BUILD_TOOL
diff --git a/src/buildtool/storage/local_ac.tpp b/src/buildtool/storage/local_ac.tpp
index 47da88f7..835027ac 100644
--- a/src/buildtool/storage/local_ac.tpp
+++ b/src/buildtool/storage/local_ac.tpp
@@ -15,8 +15,10 @@
 #ifndef INCLUDED_SRC_BUILDTOOL_STORAGE_LOCAL_AC_TPP
 #define INCLUDED_SRC_BUILDTOOL_STORAGE_LOCAL_AC_TPP
 
-#include <tuple>  //std::ignore
+#include <tuple>    //std::ignore
+#include <utility>  // std::move
 
+#include "fmt/core.h"
 #include "src/buildtool/common/bazel_types.hpp"
 #include "src/buildtool/logging/log_level.hpp"
 #include "src/buildtool/storage/local_ac.hpp"
@@ -25,45 +27,26 @@ template <bool kDoGlobalUplink>
 auto LocalAC<kDoGlobalUplink>::StoreResult(
     bazel_re::Digest const& action_id,
     bazel_re::ActionResult const& result) const noexcept -> bool {
-    auto bytes = result.SerializeAsString();
-    auto digest = cas_.StoreBlob(bytes);
-    return (digest and
-            file_store_.AddFromBytes(NativeSupport::Unprefix(action_id.hash()),
-                                     digest->SerializeAsString()));
+    auto const cas_key = WriteAction(result);
+    return cas_key.has_value() and WriteActionKey(action_id, *cas_key);
 }
 
 template <bool kDoGlobalUplink>
 auto LocalAC<kDoGlobalUplink>::CachedResult(bazel_re::Digest const& action_id)
     const noexcept -> std::optional<bazel_re::ActionResult> {
-    auto id = NativeSupport::Unprefix(action_id.hash());
-    auto entry_path = file_store_.GetPath(id);
-
-    if constexpr (kDoGlobalUplink) {
-        // Uplink any existing action-cache entries in storage generations
-        std::ignore = uplinker_.UplinkActionCacheEntry(action_id);
-    }
-
-    auto const entry =
-        FileSystemManager::ReadFile(entry_path, ObjectType::File);
-    if (not entry) {
-        logger_->Emit(LogLevel::Debug,
-                      "Cache miss, entry not found {}",
-                      entry_path.string());
-        return std::nullopt;
-    }
-    bazel_re::Digest digest{};
-    if (not digest.ParseFromString(*entry)) {
-        logger_->Emit(
-            LogLevel::Warning, "Parsing cache entry failed for action {}", id);
+    auto const cas_key = ReadActionKey(action_id);
+    if (not cas_key) {
+        logger_->Emit(LogLevel::Warning, cas_key.error());
         return std::nullopt;
     }
-    auto result = ReadResult(digest);
+    auto result = ReadAction(*cas_key);
     if (not result) {
         logger_->Emit(LogLevel::Warning,
                       "Parsing action result failed for action {}",
-                      id);
+                      NativeSupport::Unprefix(action_id.hash()));
+        return std::nullopt;
     }
-    return result;
+    return std::move(result);
 }
 
 template <bool kDoGlobalUplink>
@@ -72,24 +55,19 @@ requires(kIsLocalGeneration) auto LocalAC<kDoGlobalUplink>::LocalUplinkEntry(
     LocalGenerationAC const& latest,
     bazel_re::Digest const& action_id) const noexcept -> bool {
     // Determine action cache key path in latest generation.
-    auto key_digest = NativeSupport::Unprefix(action_id.hash());
+    auto const key_digest = NativeSupport::Unprefix(action_id.hash());
     if (FileSystemManager::IsFile(latest.file_store_.GetPath(key_digest))) {
         return true;
     }
 
     // Read cache key
-    auto cache_key = file_store_.GetPath(key_digest);
-    auto content = FileSystemManager::ReadFile(cache_key);
-    if (not content) {
+    auto const cas_key = ReadActionKey(action_id);
+    if (not cas_key) {
         return false;
     }
 
     // Read result (cache value)
-    bazel_re::Digest result_digest{};
-    if (not result_digest.ParseFromString(*content)) {
-        return false;
-    }
-    auto result = ReadResult(result_digest);
+    auto const result = ReadAction(*cas_key);
     if (not result) {
         return false;
     }
@@ -125,25 +103,76 @@ requires(kIsLocalGeneration) auto LocalAC<kDoGlobalUplink>::LocalUplinkEntry(
 
     // Uplink result (cache value)
     if (not cas_.LocalUplinkBlob(latest.cas_,
-                                 result_digest,
+                                 *cas_key,
                                  /*is_executable=*/false)) {
         return false;
     }
 
+    auto const ac_entry_path = file_store_.GetPath(key_digest);
     // Uplink cache key
-    return latest.file_store_.AddFromFile(
-        key_digest, cache_key, /*is_owner=*/true);
+    return latest.file_store_.AddFromFile(key_digest,
+                                          ac_entry_path,
+                                          /*is_owner=*/true);
+}
+
+template <bool kDoGlobalUplink>
+auto LocalAC<kDoGlobalUplink>::WriteActionKey(
+    bazel_re::Digest const& action_id,
+    bazel_re::Digest const& cas_key) const noexcept -> bool {
+    return file_store_.AddFromBytes(NativeSupport::Unprefix(action_id.hash()),
+                                    cas_key.SerializeAsString());
+}
+
+template <bool kDoGlobalUplink>
+auto LocalAC<kDoGlobalUplink>::ReadActionKey(bazel_re::Digest const& action_id)
+    const noexcept -> expected<bazel_re::Digest, std::string> {
+    auto const key_path =
+        file_store_.GetPath(NativeSupport::Unprefix(action_id.hash()));
+
+    if constexpr (kDoGlobalUplink) {
+        // Uplink any existing action-cache entries in storage generations
+        std::ignore = uplinker_.UplinkActionCacheEntry(action_id);
+    }
+
+    auto const key_content =
+        FileSystemManager::ReadFile(key_path, ObjectType::File);
+    if (not key_content) {
+        return unexpected{
+            fmt::format("Cache miss, entry not found {}", key_path.string())};
+    }
+
+    bazel_re::Digest action_key{};
+    if (not action_key.ParseFromString(*key_content)) {
+        return unexpected{
+            fmt::format("Parsing cache entry failed for action {}",
+                        NativeSupport::Unprefix(action_id.hash()))};
+    }
+    return std::move(action_key);
 }
 
 template <bool kDoGlobalUplink>
-auto LocalAC<kDoGlobalUplink>::ReadResult(bazel_re::Digest const& digest)
+auto LocalAC<kDoGlobalUplink>::WriteAction(bazel_re::ActionResult const& action)
+    const noexcept -> std::optional<bazel_re::Digest> {
+    return cas_.StoreBlob(action.SerializeAsString(),
+                          /*is_executable=*/false);
+}
+
+template <bool kDoGlobalUplink>
+auto LocalAC<kDoGlobalUplink>::ReadAction(bazel_re::Digest const& cas_key)
     const noexcept -> std::optional<bazel_re::ActionResult> {
-    bazel_re::ActionResult result{};
-    if (auto src_path = cas_.BlobPath(digest, /*is_executable=*/false)) {
-        auto const bytes = FileSystemManager::ReadFile(*src_path);
-        if (bytes.has_value() and result.ParseFromString(*bytes)) {
-            return result;
-        }
+    auto const action_path = cas_.BlobPath(cas_key,
+                                           /*is_executable=*/false);
+    if (not action_path) {
+        return std::nullopt;
+    }
+    auto const action_content = FileSystemManager::ReadFile(*action_path);
+    if (not action_content) {
+        return std::nullopt;
+    }
+
+    bazel_re::ActionResult action{};
+    if (action.ParseFromString(*action_content)) {
+        return std::move(action);
     }
     return std::nullopt;
 }