serve target: Differentiate between fatal and non-fatal orchestrated builds

...by increasing granularity in client-side reporting. This allows
to correctly continue with builds of local targets if the serve
endpoint does not have the requested target, as well as improve
the reporting for users on failure.

6 files changed, 163 insertions, 62 deletions

diff --git a/src/buildtool/build_engine/target_map/absent_target_map.cpp b/src/buildtool/build_engine/target_map/absent_target_map.cpp
index 39e3d3bb..917ddd2a 100644
--- a/src/buildtool/build_engine/target_map/absent_target_map.cpp
+++ b/src/buildtool/build_engine/target_map/absent_target_map.cpp
@@ -52,7 +52,7 @@ auto BuildMaps::Target::CreateAbsentTargetMap(
             key.target.GetNamedTarget().name);
         if (!flexible_vars) {
             (*logger)(fmt::format("Failed to obtain flexible config variables "
-                                  "for absent target \"{}\"",
+                                  "for absent target {}",
                                   key.target.ToString()),
                       /*fatal=*/true);
             return;
@@ -89,8 +89,36 @@ auto BuildMaps::Target::CreateAbsentTargetMap(
                         key.target.ToString());
             exports_progress->TaskTracker().Start(
                 target_cache_key->Id().ToString());
-            target_cache_value =
-                ServeApi::ServeTarget(*target_cache_key, *repo_key);
+            auto res = ServeApi::ServeTarget(*target_cache_key, *repo_key);
+            // process response from serve endpoint
+            if (not res) {
+                // report target not found
+                (*logger)(fmt::format("Absent target {} was not found on serve "
+                                      "endpoint",
+                                      key.target.ToString()),
+                          /*fatal=*/true);
+                return;
+            }
+            if (res->index() == 0) {
+                (*logger)(
+                    fmt::format("Failure to remotely analyse or build absent "
+                                "target {}\nDetailed log available on the "
+                                "remote-execution endpoint as blob {}",
+                                key.target.ToString(),
+                                std::get<0>(*res)),
+                    /*fatal=*/true);
+                return;
+            }
+            if (res->index() == 1) {
+                (*logger)(fmt::format("While querying serve endpoint for "
+                                      "absent export target {}:\n{}",
+                                      key.target.ToString(),
+                                      std::get<1>(*res)),
+                          /*fatal=*/true);
+                return;
+            }
+            // index == 2
+            target_cache_value = std::get<2>(*res);
             exports_progress->TaskTracker().Stop(
                 target_cache_key->Id().ToString());
             from_just_serve = true;
diff --git a/src/buildtool/build_engine/target_map/export.cpp b/src/buildtool/build_engine/target_map/export.cpp
index 3ed86dd1..b8b5b1db 100644
--- a/src/buildtool/build_engine/target_map/export.cpp
+++ b/src/buildtool/build_engine/target_map/export.cpp
@@ -135,11 +135,43 @@ void ExportRule(
             auto task = fmt::format(
                 "[{},{}]", key.target.ToString(), effective_config.ToString());
             exports_progress->TaskTracker().Start(task);
-
-            target_cache_value =
-                ServeApi::ServeTarget(*target_cache_key, *repo_key);
+            auto res = ServeApi::ServeTarget(*target_cache_key, *repo_key);
+            // process response from serve endpoint
+            if (not res) {
+                // target not found: log to performance, and continue
+                Logger::Log(LogLevel::Performance,
+                            "Export target {} not known to serve endpoint",
+                            key.target.ToString());
+            }
+            else {
+                if (res->index() == 0) {
+                    // target found but failed to analyse/build: this should be
+                    // a fatal error for the local build too
+                    (*logger)(
+                        fmt::format("Failure to remotely analyse or build "
+                                    "target {}\nDetailed log available on the "
+                                    "remote-execution endpoint as blob {}",
+                                    key.target.ToString(),
+                                    std::get<0>(*res)),
+                        /*fatal=*/true);
+                    return;
+                }
+                if (res->index() == 1) {
+                    // some other failure occurred while querying the serve
+                    // endpoint; log to debug and continue locally
+                    Logger::Log(LogLevel::Debug,
+                                "While querying serve endpoint for export "
+                                "target {}:\n{}",
+                                key.target.ToString(),
+                                std::get<1>(*res));
+                }
+                else {
+                    // index == 2
+                    target_cache_value = std::get<2>(*res);
+                    from_just_serve = true;
+                }
+            }
             exports_progress->TaskTracker().Stop(task);
-            from_just_serve = true;
         }
 #endif  // BOOTSTRAP_BUILD_TOOL
 
diff --git a/src/buildtool/serve_api/remote/TARGETS b/src/buildtool/serve_api/remote/TARGETS
index 1e875bbd..e039cfdb 100644
--- a/src/buildtool/serve_api/remote/TARGETS
+++ b/src/buildtool/serve_api/remote/TARGETS
@@ -58,6 +58,7 @@
   , "private-deps":
     [ ["src/buildtool/common/remote", "client_common"]
     , ["src/buildtool/common", "bazel_types"]
+    , ["@", "fmt", "", "fmt"]
     , ["@", "json", "", "json"]
     ]
   }
diff --git a/src/buildtool/serve_api/remote/serve_api.hpp b/src/buildtool/serve_api/remote/serve_api.hpp
index 219fe7ab..055cf78b 100644
--- a/src/buildtool/serve_api/remote/serve_api.hpp
+++ b/src/buildtool/serve_api/remote/serve_api.hpp
@@ -111,7 +111,7 @@ class ServeApi final {
 
     [[nodiscard]] static auto ServeTarget(const TargetCacheKey& key,
                                           const std::string& repo_key) noexcept
-        -> std::optional<std::pair<TargetCacheEntry, Artifact::ObjectInfo>> {
+        -> std::optional<serve_target_result_t> {
         return Instance().tc_->ServeTarget(key, repo_key);
     }
 
diff --git a/src/buildtool/serve_api/remote/target_client.cpp b/src/buildtool/serve_api/remote/target_client.cpp
index bee31249..d64f24c8 100644
--- a/src/buildtool/serve_api/remote/target_client.cpp
+++ b/src/buildtool/serve_api/remote/target_client.cpp
@@ -17,6 +17,7 @@
 #include <exception>
 #include <utility>
 
+#include "fmt/core.h"
 #include "nlohmann/json.hpp"
 #include "src/buildtool/common/bazel_types.hpp"
 #include "src/buildtool/common/remote/client_common.hpp"
@@ -29,23 +30,22 @@ TargetClient::TargetClient(std::string const& server, Port port) noexcept {
 
 auto TargetClient::ServeTarget(const TargetCacheKey& key,
                                const std::string& repo_key) noexcept
-    -> std::optional<std::pair<TargetCacheEntry, Artifact::ObjectInfo>> {
+    -> std::optional<serve_target_result_t> {
     // make sure the blob containing the key is in the remote cas
     if (!local_api_->RetrieveToCas({key.Id()}, &*remote_api_)) {
-        logger_.Emit(LogLevel::Performance,
-                     "failed to retrieve to remote cas ObjectInfo {}",
-                     key.Id().ToString());
-        return std::nullopt;
+        return serve_target_result_t{
+            std::in_place_index<1>,
+            fmt::format("Failed to retrieve to remote cas ObjectInfo {}",
+                        key.Id().ToString())};
     }
     // make sure the repository configuration blob is in the remote cas
     if (!local_api_->RetrieveToCas(
             {Artifact::ObjectInfo{.digest = ArtifactDigest{repo_key, 0, false},
                                   .type = ObjectType::File}},
             &*remote_api_)) {
-        logger_.Emit(LogLevel::Performance,
-                     "failed to retrieve to remote cas blob {}",
-                     repo_key);
-        return std::nullopt;
+        return serve_target_result_t{
+            std::in_place_index<1>,
+            fmt::format("Failed to retrieve to remote cas blob {}", repo_key)};
     }
 
     // add target cache key to request
@@ -72,27 +72,27 @@ auto TargetClient::ServeTarget(const TargetCacheKey& key,
             dispatch_list.push_back(entry);
         }
     } catch (std::exception const& ex) {
-        logger_.Emit(LogLevel::Performance,
-                     "populating dispatch JSON array failed with:\n{}",
-                     ex.what());
-        return std::nullopt;
+        return serve_target_result_t{
+            std::in_place_index<1>,
+            fmt::format("Populating dispatch JSON array failed with:\n{}",
+                        ex.what())};
     }
 
     auto dispatch_dgst =
         Storage::Instance().CAS().StoreBlob(dispatch_list.dump(2));
     if (not dispatch_dgst) {
-        logger_.Emit(LogLevel::Performance,
-                     "failed to store blob {} to local cas",
-                     dispatch_list.dump(2));
-        return std::nullopt;
+        return serve_target_result_t{
+            std::in_place_index<1>,
+            fmt::format("Failed to store blob {} to local cas",
+                        dispatch_list.dump(2))};
     }
     auto const& dispatch_info = Artifact::ObjectInfo{
         .digest = ArtifactDigest{*dispatch_dgst}, .type = ObjectType::File};
     if (!local_api_->RetrieveToCas({dispatch_info}, &*remote_api_)) {
-        logger_.Emit(LogLevel::Performance,
-                     "failed to upload blob {} to remote cas",
-                     dispatch_info.ToString());
-        return std::nullopt;
+        return serve_target_result_t{
+            std::in_place_index<1>,
+            fmt::format("Failed to upload blob {} to remote cas",
+                        dispatch_info.ToString())};
     }
     request.mutable_dispatch_info()->CopyFrom(*dispatch_dgst);
 
@@ -101,39 +101,67 @@ auto TargetClient::ServeTarget(const TargetCacheKey& key,
     justbuild::just_serve::ServeTargetResponse response;
     auto const& status = stub_->ServeTarget(&context, request, &response);
     if (!status.ok()) {
-        LogStatus(&logger_, LogLevel::Performance, status);
-        return std::nullopt;
+        return serve_target_result_t{std::in_place_index<1>,
+                                     StatusString(status)};
     }
 
-    auto const& target_value_dgst = ArtifactDigest{response.target_value()};
-    auto const& obj_info = Artifact::ObjectInfo{.digest = target_value_dgst,
-                                                .type = ObjectType::File};
-    if (!local_api_->IsAvailable(target_value_dgst)) {
-        if (!remote_api_->RetrieveToCas({obj_info}, &*local_api_)) {
-            logger_.Emit(LogLevel::Performance,
-                         "failed to retrieve blob {} from remote cas",
-                         obj_info.ToString());
-            return std::nullopt;
+    // differentiate status codes
+    switch (status.error_code()) {
+        case grpc::StatusCode::OK: {
+            // if log has been set, pass it along as index 0
+            if (response.has_log()) {
+                return serve_target_result_t{
+                    std::in_place_index<0>,
+                    ArtifactDigest(response.log()).hash()};
+            }
+            // if no log has been set, it must have the target cache value
+            if (not response.has_target_value()) {
+                return serve_target_result_t{
+                    std::in_place_index<1>,
+                    "Serve endpoint failed to set expected response field"};
+            }
+            auto const& target_value_dgst =
+                ArtifactDigest{response.target_value()};
+            auto const& obj_info = Artifact::ObjectInfo{
+                .digest = target_value_dgst, .type = ObjectType::File};
+            if (!local_api_->IsAvailable(target_value_dgst)) {
+                if (!remote_api_->RetrieveToCas({obj_info}, &*local_api_)) {
+                    return serve_target_result_t{
+                        std::in_place_index<1>,
+                        fmt::format(
+                            "Failed to retrieve blob {} from remote cas",
+                            obj_info.ToString())};
+                }
+            }
+            auto const& target_value_str =
+                local_api_->RetrieveToMemory(obj_info);
+            if (!target_value_str) {
+                return serve_target_result_t{
+                    std::in_place_index<1>,
+                    fmt::format("Failed to retrieve blob {} from local cas",
+                                obj_info.ToString())};
+            }
+            try {
+                auto const& result = TargetCacheEntry::FromJson(
+                    nlohmann::json::parse(*target_value_str));
+                // return the target cache value information
+                return serve_target_result_t{std::in_place_index<2>,
+                                             std::make_pair(result, obj_info)};
+            } catch (std::exception const& ex) {
+                return serve_target_result_t{
+                    std::in_place_index<1>,
+                    fmt::format("Parsing target cache value failed with:\n{}",
+                                ex.what())};
+            }
         }
+        case grpc::StatusCode::NOT_FOUND: {
+            return std::nullopt;  // this might allow a local build to continue
+        }
+        default:;  // fallthrough
     }
-
-    auto const& target_value_str = local_api_->RetrieveToMemory(obj_info);
-    if (!target_value_str) {
-        logger_.Emit(LogLevel::Performance,
-                     "failed to retrieve blob {} from local cas",
-                     obj_info.ToString());
-        return std::nullopt;
-    }
-    try {
-        auto const& result = TargetCacheEntry::FromJson(
-            nlohmann::json::parse(*target_value_str));
-        return std::make_pair(result, obj_info);
-    } catch (std::exception const& ex) {
-        logger_.Emit(LogLevel::Performance,
-                     "parsing target cache value failed with:\n{}",
-                     ex.what());
-        return std::nullopt;
-    }
+    return serve_target_result_t{
+        std::in_place_index<1>,
+        fmt::format("Serve endpoint failed with:\n{}", status.error_message())};
 }
 
 auto TargetClient::ServeTargetVariables(std::string const& target_root_id,
diff --git a/src/buildtool/serve_api/remote/target_client.hpp b/src/buildtool/serve_api/remote/target_client.hpp
index c9540cb0..948e9337 100644
--- a/src/buildtool/serve_api/remote/target_client.hpp
+++ b/src/buildtool/serve_api/remote/target_client.hpp
@@ -19,6 +19,7 @@
 #include <optional>
 #include <string>
 #include <utility>
+#include <variant>
 #include <vector>
 
 #include "gsl/gsl"
@@ -31,6 +32,16 @@
 #include "src/buildtool/storage/target_cache_entry.hpp"
 #include "src/buildtool/storage/target_cache_key.hpp"
 
+/// \brief Result union for the ServeTarget request.
+/// Index 0 will contain the hash of the blob containing the logged
+/// analysis/build failure received from the endpoint, as a string.
+/// Index 1 will contain any other failure message, as a string.
+/// Index 2 will contain the target cache value information on success.
+using serve_target_result_t =
+    std::variant<std::string,
+                 std::string,
+                 std::pair<TargetCacheEntry, Artifact::ObjectInfo>>;
+
 /// Implements client side for Target service defined in:
 /// src/buildtool/serve_api/serve_service/just_serve.proto
 class TargetClient {
@@ -39,12 +50,13 @@ class TargetClient {
 
     /// \brief Retrieve the pair of TargetCacheEntry and ObjectInfo associated
     /// to the given key.
-    /// \param[in] key The TargetCacheKey of an export target
-    /// \param[in] repo_key The RepositoryKey to upload as precondition
-    /// \returns Pair of cache entry and its object info on success or nullopt.
+    /// \param[in] key The TargetCacheKey of an export target.
+    /// \param[in] repo_key The RepositoryKey to upload as precondition.
+    /// \returns A correspondingly populated result union, or nullopt if remote
+    /// reported that the target was not found.
     [[nodiscard]] auto ServeTarget(const TargetCacheKey& key,
                                    const std::string& repo_key) noexcept
-        -> std::optional<std::pair<TargetCacheEntry, Artifact::ObjectInfo>>;
+        -> std::optional<serve_target_result_t>;
 
     /// \brief Retrieve the flexible config variables of an export target.
     /// \param[in] target_root_id Hash of target-level root tree.