execution_response: Allow failures to be reported while populating

As populating the containers from remote response only takes place
once, no assumptions should be made that this cannot fail (for
example if wrong or invalid entries were produced). Instead, return
error messages on failure to callers that can log accordingly.

4 files changed, 119 insertions, 94 deletions

diff --git a/test/buildtool/execution_api/common/api_test.hpp b/test/buildtool/execution_api/common/api_test.hpp
index 4b84d66a..d00eec7b 100644
--- a/test/buildtool/execution_api/common/api_test.hpp
+++ b/test/buildtool/execution_api/common/api_test.hpp
@@ -164,26 +164,28 @@ using ExecProps = std::map<std::string, std::string>;
         action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput);
 
         // run execution
-        auto response = action->Execute();
+        auto const response = action->Execute();
         REQUIRE(response);
 
         // verify result
-        auto artifacts = response->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(artifacts.at(output_path).digest == test_digest);
+        auto const artifacts = response->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(artifacts.value()->at(output_path).digest == test_digest);
 
         if (is_hermetic) {
             CHECK(not response->IsCached());
 
             SECTION("Rerun execution to verify caching") {
                 // run execution
-                auto response = action->Execute();
+                auto const response = action->Execute();
                 REQUIRE(response);
 
                 // verify result
-                auto artifacts = response->Artifacts();
-                REQUIRE(artifacts.contains(output_path));
-                CHECK(artifacts.at(output_path).digest == test_digest);
+                auto const artifacts = response->Artifacts();
+                REQUIRE(artifacts.has_value());
+                REQUIRE(artifacts.value()->contains(output_path));
+                CHECK(artifacts.value()->at(output_path).digest == test_digest);
                 CHECK(response->IsCached());
             }
         }
@@ -193,24 +195,25 @@ using ExecProps = std::map<std::string, std::string>;
         action->SetCacheFlag(IExecutionAction::CacheFlag::DoNotCacheOutput);
 
         // run execution
-        auto response = action->Execute();
+        auto const response = action->Execute();
         REQUIRE(response);
 
         // verify result
-        auto artifacts = response->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(artifacts.at(output_path).digest == test_digest);
+        auto const artifacts = response->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(artifacts.value()->at(output_path).digest == test_digest);
         CHECK(not response->IsCached());
 
         SECTION("Rerun execution to verify caching") {
             // run execution
-            auto response = action->Execute();
+            auto const response = action->Execute();
             REQUIRE(response);
 
             // verify result
-            auto artifacts = response->Artifacts();
-            REQUIRE(artifacts.contains(output_path));
-            CHECK(artifacts.at(output_path).digest == test_digest);
+            auto const artifacts = response->Artifacts();
+            REQUIRE(artifacts.value()->contains(output_path));
+            CHECK(artifacts.value()->at(output_path).digest == test_digest);
             CHECK(not response->IsCached());
         }
     }
@@ -254,26 +257,28 @@ using ExecProps = std::map<std::string, std::string>;
         action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput);
 
         // run execution
-        auto response = action->Execute();
+        auto const response = action->Execute();
         REQUIRE(response);
 
         // verify result
-        auto artifacts = response->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(artifacts.at(output_path).digest == test_digest);
+        auto const artifacts = response->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(artifacts.value()->at(output_path).digest == test_digest);
 
         if (is_hermetic) {
             CHECK(not response->IsCached());
 
             SECTION("Rerun execution to verify caching") {
                 // run execution
-                auto response = action->Execute();
+                auto const response = action->Execute();
                 REQUIRE(response);
 
                 // verify result
-                auto artifacts = response->Artifacts();
-                REQUIRE(artifacts.contains(output_path));
-                CHECK(artifacts.at(output_path).digest == test_digest);
+                auto const artifacts = response->Artifacts();
+                REQUIRE(artifacts.has_value());
+                REQUIRE(artifacts.value()->contains(output_path));
+                CHECK(artifacts.value()->at(output_path).digest == test_digest);
                 CHECK(response->IsCached());
             }
         }
@@ -283,24 +288,26 @@ using ExecProps = std::map<std::string, std::string>;
         action->SetCacheFlag(IExecutionAction::CacheFlag::DoNotCacheOutput);
 
         // run execution
-        auto response = action->Execute();
+        auto const response = action->Execute();
         REQUIRE(response);
 
         // verify result
-        auto artifacts = response->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(artifacts.at(output_path).digest == test_digest);
+        auto const artifacts = response->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(artifacts.value()->at(output_path).digest == test_digest);
         CHECK(not response->IsCached());
 
         SECTION("Rerun execution to verify caching") {
             // run execution
-            auto response = action->Execute();
+            auto const response = action->Execute();
             REQUIRE(response);
 
             // verify result
-            auto artifacts = response->Artifacts();
-            REQUIRE(artifacts.contains(output_path));
-            CHECK(artifacts.at(output_path).digest == test_digest);
+            auto const artifacts = response->Artifacts();
+            REQUIRE(artifacts.has_value());
+            REQUIRE(artifacts.value()->contains(output_path));
+            CHECK(artifacts.value()->at(output_path).digest == test_digest);
             CHECK(not response->IsCached());
         }
     }
@@ -335,27 +342,29 @@ using ExecProps = std::map<std::string, std::string>;
         action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput);
 
         // run execution
-        auto response = action->Execute();
+        auto const response = action->Execute();
         REQUIRE(response);
 
         // verify result
         CHECK(response->ExitCode() == 1);
-        auto artifacts = response->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(artifacts.at(output_path).digest == test_digest);
+        auto const artifacts = response->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(artifacts.value()->at(output_path).digest == test_digest);
 
         CHECK(not response->IsCached());
 
         SECTION("Rerun execution to verify that non-zero actions are rerun") {
             // run execution
-            auto response = action->Execute();
+            auto const response = action->Execute();
             REQUIRE(response);
 
             // verify result
             CHECK(response->ExitCode() == 1);
-            auto artifacts = response->Artifacts();
-            REQUIRE(artifacts.contains(output_path));
-            CHECK(artifacts.at(output_path).digest == test_digest);
+            auto const artifacts = response->Artifacts();
+            REQUIRE(artifacts.has_value());
+            REQUIRE(artifacts.value()->contains(output_path));
+            CHECK(artifacts.value()->at(output_path).digest == test_digest);
             CHECK(not response->IsCached());
         }
     }
@@ -364,14 +373,15 @@ using ExecProps = std::map<std::string, std::string>;
         action->SetCacheFlag(IExecutionAction::CacheFlag::DoNotCacheOutput);
 
         // run execution
-        auto response = action->Execute();
+        auto const response = action->Execute();
         REQUIRE(response);
 
         // verify result
         CHECK(response->ExitCode() == 1);
-        auto artifacts = response->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(artifacts.at(output_path).digest == test_digest);
+        auto const artifacts = response->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(artifacts.value()->at(output_path).digest == test_digest);
         CHECK(not response->IsCached());
 
         SECTION("Rerun execution to verify non-zero actions are not cached") {
@@ -381,9 +391,10 @@ using ExecProps = std::map<std::string, std::string>;
 
             // verify result
             CHECK(response->ExitCode() == 1);
-            auto artifacts = response->Artifacts();
-            REQUIRE(artifacts.contains(output_path));
-            CHECK(artifacts.at(output_path).digest == test_digest);
+            auto const artifacts = response->Artifacts();
+            REQUIRE(artifacts.has_value());
+            REQUIRE(artifacts.value()->contains(output_path));
+            CHECK(artifacts.value()->at(output_path).digest == test_digest);
             CHECK(not response->IsCached());
         }
     }
@@ -427,7 +438,7 @@ using ExecProps = std::map<std::string, std::string>;
     action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput);
 
     // run execution
-    auto response = action->Execute();
+    auto const response = action->Execute();
     REQUIRE(response);
 
     // verify result
@@ -437,10 +448,11 @@ using ExecProps = std::map<std::string, std::string>;
         CHECK_FALSE(response->IsCached());
     }
 
-    auto artifacts = response->Artifacts();
-    REQUIRE_FALSE(artifacts.empty());
+    auto const artifacts = response->Artifacts();
+    REQUIRE(artifacts.has_value());
+    REQUIRE_FALSE(artifacts.value()->empty());
 
-    auto info = artifacts.begin()->second;
+    auto info = artifacts.value()->begin()->second;
 
     SECTION("retrieve via same API object") {
         auto out_path = GetTestDir(test_name) / "out1";
@@ -500,7 +512,7 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory,
     action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput);
 
     // run execution
-    auto response = action->Execute();
+    auto const response = action->Execute();
     REQUIRE(response);
 
     // verify result
@@ -510,10 +522,11 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory,
         CHECK_FALSE(response->IsCached());
     }
 
-    auto artifacts = response->Artifacts();
-    REQUIRE_FALSE(artifacts.empty());
+    auto const artifacts = response->Artifacts();
+    REQUIRE(artifacts.has_value());
+    REQUIRE_FALSE(artifacts.value()->empty());
 
-    auto info = artifacts.begin()->second;
+    auto info = artifacts.value()->begin()->second;
 
     SECTION("retrieve via same API object") {
         auto out_path = GetTestDir(test_name) / "out1";
@@ -569,7 +582,7 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory,
     action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput);
 
     // run execution
-    auto response = action->Execute();
+    auto const response = action->Execute();
     REQUIRE(response);
 
     // verify result
@@ -579,16 +592,17 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory,
         CHECK_FALSE(response->IsCached());
     }
 
-    auto artifacts = response->Artifacts();
-    REQUIRE_FALSE(artifacts.empty());
+    auto const artifacts = response->Artifacts();
+    REQUIRE(artifacts.has_value());
+    REQUIRE_FALSE(artifacts.value()->empty());
 
     std::vector<std::filesystem::path> paths{};
     std::vector<Artifact::ObjectInfo> infos{};
 
     SECTION("retrieve via same API object") {
         auto out_path = GetTestDir(test_name) / "out1";
-        std::for_each(artifacts.begin(),
-                      artifacts.end(),
+        std::for_each(artifacts.value()->begin(),
+                      artifacts.value()->end(),
                       [&out_path, &paths, &infos](auto const& entry) {
                           paths.emplace_back(out_path / entry.first);
                           infos.emplace_back(entry.second);
@@ -602,8 +616,8 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory,
     SECTION("retrieve from new API object but same endpoint") {
         auto second_api = api_factory();
         auto out_path = GetTestDir(test_name) / "out2";
-        std::for_each(artifacts.begin(),
-                      artifacts.end(),
+        std::for_each(artifacts.value()->begin(),
+                      artifacts.value()->end(),
                       [&out_path, &paths, &infos](auto const& entry) {
                           paths.emplace_back(out_path / entry.first);
                           infos.emplace_back(entry.second);
@@ -638,26 +652,28 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory,
         action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput);
 
         // run execution
-        auto response = action->Execute();
+        auto const response = action->Execute();
         REQUIRE(response);
 
         // verify result
-        auto artifacts = response->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(IsTreeObject(artifacts.at(output_path).type));
+        auto const artifacts = response->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(IsTreeObject(artifacts.value()->at(output_path).type));
 
         if (is_hermetic) {
             CHECK(not response->IsCached());
 
             SECTION("Rerun execution to verify caching") {
                 // run execution
-                auto response = action->Execute();
+                auto const response = action->Execute();
                 REQUIRE(response);
 
                 // verify result
-                auto artifacts = response->Artifacts();
-                REQUIRE(artifacts.contains(output_path));
-                CHECK(IsTreeObject(artifacts.at(output_path).type));
+                auto const artifacts = response->Artifacts();
+                REQUIRE(artifacts.has_value());
+                REQUIRE(artifacts.value()->contains(output_path));
+                CHECK(IsTreeObject(artifacts.value()->at(output_path).type));
                 CHECK(response->IsCached());
             }
         }
@@ -667,24 +683,26 @@ TestRetrieveFileAndSymlinkWithSameContentToPath(ApiFactory const& api_factory,
         action->SetCacheFlag(IExecutionAction::CacheFlag::DoNotCacheOutput);
 
         // run execution
-        auto response = action->Execute();
+        auto const response = action->Execute();
         REQUIRE(response);
 
         // verify result
-        auto artifacts = response->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(IsTreeObject(artifacts.at(output_path).type));
+        auto const artifacts = response->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(IsTreeObject(artifacts.value()->at(output_path).type));
         CHECK(not response->IsCached());
 
         SECTION("Rerun execution to verify caching") {
             // run execution
-            auto response = action->Execute();
+            auto const response = action->Execute();
             REQUIRE(response);
 
             // verify result
-            auto artifacts = response->Artifacts();
-            REQUIRE(artifacts.contains(output_path));
-            CHECK(IsTreeObject(artifacts.at(output_path).type));
+            auto const artifacts = response->Artifacts();
+            REQUIRE(artifacts.has_value());
+            REQUIRE(artifacts.value()->contains(output_path));
+            CHECK(IsTreeObject(artifacts.value()->at(output_path).type));
             CHECK(not response->IsCached());
         }
     }
diff --git a/test/buildtool/execution_api/local/local_execution.test.cpp b/test/buildtool/execution_api/local/local_execution.test.cpp
index 120100de..28fd03c6 100644
--- a/test/buildtool/execution_api/local/local_execution.test.cpp
+++ b/test/buildtool/execution_api/local/local_execution.test.cpp
@@ -211,9 +211,10 @@ TEST_CASE("LocalExecution: No input, create output", "[execution_api]") {
 
         // verify result
         CHECK_FALSE(output->IsCached());
-        auto artifacts = output->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(artifacts.at(output_path).digest == test_digest);
+        auto const artifacts = output->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(artifacts.value()->at(output_path).digest == test_digest);
 
         // ensure result IS in cache
         output = action->Execute(nullptr);
@@ -229,9 +230,10 @@ TEST_CASE("LocalExecution: No input, create output", "[execution_api]") {
 
         // verify result
         CHECK_FALSE(output->IsCached());
-        auto artifacts = output->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(artifacts.at(output_path).digest == test_digest);
+        auto const artifacts = output->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(artifacts.value()->at(output_path).digest == test_digest);
 
         // ensure result IS STILL NOT in cache
         output = action->Execute(nullptr);
@@ -290,9 +292,10 @@ TEST_CASE("LocalExecution: One input copied to output", "[execution_api]") {
 
         // verify result
         CHECK_FALSE(output->IsCached());
-        auto artifacts = output->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(artifacts.at(output_path).digest == test_digest);
+        auto const artifacts = output->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(artifacts.value()->at(output_path).digest == test_digest);
 
         // ensure result IS in cache
         output = action->Execute(nullptr);
@@ -308,9 +311,10 @@ TEST_CASE("LocalExecution: One input copied to output", "[execution_api]") {
 
         // verify result
         CHECK_FALSE(output->IsCached());
-        auto artifacts = output->Artifacts();
-        REQUIRE(artifacts.contains(output_path));
-        CHECK(artifacts.at(output_path).digest == test_digest);
+        auto const artifacts = output->Artifacts();
+        REQUIRE(artifacts.has_value());
+        REQUIRE(artifacts.value()->contains(output_path));
+        CHECK(artifacts.value()->at(output_path).digest == test_digest);
 
         // ensure result IS STILL NOT in cache
         output = action->Execute(nullptr);
diff --git a/test/buildtool/execution_engine/executor/TARGETS b/test/buildtool/execution_engine/executor/TARGETS
index bb86c6d8..343b83eb 100644
--- a/test/buildtool/execution_engine/executor/TARGETS
+++ b/test/buildtool/execution_engine/executor/TARGETS
@@ -43,6 +43,7 @@
     , ["@", "src", "src/buildtool/execution_engine/executor", "executor"]
     , ["@", "src", "src/buildtool/progress_reporting", "progress"]
     , ["@", "src", "src/buildtool/crypto", "hash_function"]
+    , ["@", "src", "src/utils/cpp", "expected"]
     , ["", "catch-main"]
     , ["@", "catch2", "", "catch2"]
     , ["@", "gsl", "", "gsl"]
diff --git a/test/buildtool/execution_engine/executor/executor.test.cpp b/test/buildtool/execution_engine/executor/executor.test.cpp
index 48bcf082..e2d62342 100644
--- a/test/buildtool/execution_engine/executor/executor.test.cpp
+++ b/test/buildtool/execution_engine/executor/executor.test.cpp
@@ -38,6 +38,7 @@
 #include "src/buildtool/execution_engine/executor/context.hpp"
 #include "src/buildtool/file_system/file_system_manager.hpp"
 #include "src/buildtool/progress_reporting/progress.hpp"
+#include "src/utils/cpp/expected.hpp"
 #include "test/utils/executor/test_api_bundle.hpp"
 #include "test/utils/hermeticity/test_hash_function_type.hpp"
 
@@ -97,16 +98,17 @@ class TestResponse : public IExecutionResponse {
         static const std::string kEmptyHash;
         return kEmptyHash;
     }
-    [[nodiscard]] auto Artifacts() noexcept -> ArtifactInfos const& final {
+    [[nodiscard]] auto Artifacts() noexcept
+        -> expected<gsl::not_null<ArtifactInfos const*>, std::string> final {
         if (not populated_) {
             Populate();
         }
-        return artifacts_;
+        return gsl::not_null<ArtifactInfos const*>(&artifacts_);
     }
     [[nodiscard]] auto DirectorySymlinks() noexcept
-        -> DirSymlinks const& final {
+        -> expected<gsl::not_null<DirSymlinks const*>, std::string> final {
         static const DirSymlinks kEmptySymlinks{};
-        return kEmptySymlinks;
+        return gsl::not_null<DirSymlinks const*>(&kEmptySymlinks);
     }
 
   private: