serve target: Improve logic for local build failure triggers

If the serve endpoint reports an internal error, local builds
should not continue and the error message should be provided.
Similarly, if the serve endpoint promises the target cache value to
be in remote CAS, but the client cannot find or process it as
needed, then a local build again should not continue and the reason
be provided as an error message.

4 files changed, 85 insertions, 55 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 1ab385c3..bd5c17a2 100644
--- a/src/buildtool/build_engine/target_map/absent_target_map.cpp
+++ b/src/buildtool/build_engine/target_map/absent_target_map.cpp
@@ -95,30 +95,39 @@ void WithFlexibleVariables(
                       /*fatal=*/true);
             return;
         }
-        if (res->index() == 0) {
-            if (serve_failure_reporter != nullptr) {
-                (*serve_failure_reporter)(key, std::get<0>(*res));
+        switch (auto const& ind = res->index(); ind) {
+            case 0: {
+                if (serve_failure_reporter != nullptr) {
+                    (*serve_failure_reporter)(key, std::get<0>(*res));
+                }
+                // target found but failed to analyse/build: log it as fatal
+                (*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;
+            }
+            case 1:  // fallthrough
+            case 2: {
+                // Other errors, including INTERNAL: log it as fatal
+                (*logger)(fmt::format(
+                              "While querying serve endpoint for absent export "
+                              "target {}:\n{}",
+                              key.target.ToString(),
+                              ind == 1 ? std::get<1>(*res) : std::get<2>(*res)),
+                          /*fatal=*/true);
+                return;
+            }
+            default: {
+                // index == 3
+                target_cache_value = std::get<3>(*res);
+                exports_progress->TaskTracker().Stop(task);
+                from_just_serve = true;
             }
-            (*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(task);
-        from_just_serve = true;
     }
 
     if (!target_cache_value) {
diff --git a/src/buildtool/build_engine/target_map/export.cpp b/src/buildtool/build_engine/target_map/export.cpp
index e0bc36c3..e92d4c9c 100644
--- a/src/buildtool/build_engine/target_map/export.cpp
+++ b/src/buildtool/build_engine/target_map/export.cpp
@@ -160,31 +160,44 @@ void ExportRule(
                             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{}",
+                switch (res->index()) {
+                    case 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<1>(*res));
-                }
-                else {
-                    // index == 2
-                    target_cache_value = std::get<2>(*res);
-                    from_just_serve = true;
+                                std::get<0>(*res)),
+                            /*fatal=*/true);
+                        return;
+                    }
+                    case 1: {
+                        // internal failure on the serve endpoint, or failures
+                        // on the client side: local build should not continue
+                        (*logger)(fmt::format("While querying serve endpoint "
+                                              "for export target {}:\n{}",
+                                              key.target.ToString(),
+                                              std::get<1>(*res)),
+                                  /*fatal=*/true);
+                        return;
+                    }
+                    case 2: {
+                        // some other failure occurred on 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<2>(*res));
+                    } break;
+                    default: {
+                        // index == 3
+                        target_cache_value = std::get<3>(*res);
+                        from_just_serve = true;
+                    }
                 }
             }
             exports_progress->TaskTracker().Stop(task);
diff --git a/src/buildtool/serve_api/remote/target_client.cpp b/src/buildtool/serve_api/remote/target_client.cpp
index d64f24c8..67b50a84 100644
--- a/src/buildtool/serve_api/remote/target_client.cpp
+++ b/src/buildtool/serve_api/remote/target_client.cpp
@@ -100,10 +100,6 @@ auto TargetClient::ServeTarget(const TargetCacheKey& key,
     grpc::ClientContext context;
     justbuild::just_serve::ServeTargetResponse response;
     auto const& status = stub_->ServeTarget(&context, request, &response);
-    if (!status.ok()) {
-        return serve_target_result_t{std::in_place_index<1>,
-                                     StatusString(status)};
-    }
 
     // differentiate status codes
     switch (status.error_code()) {
@@ -145,7 +141,7 @@ auto TargetClient::ServeTarget(const TargetCacheKey& key,
                 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>,
+                return serve_target_result_t{std::in_place_index<3>,
                                              std::make_pair(result, obj_info)};
             } catch (std::exception const& ex) {
                 return serve_target_result_t{
@@ -154,13 +150,20 @@ auto TargetClient::ServeTarget(const TargetCacheKey& key,
                                 ex.what())};
             }
         }
+        case grpc::StatusCode::INTERNAL: {
+            return serve_target_result_t{
+                std::in_place_index<1>,
+                fmt::format(
+                    "Serve endpoint reported the fatal internal error:\n{}",
+                    status.error_message())};
+        }
         case grpc::StatusCode::NOT_FOUND: {
             return std::nullopt;  // this might allow a local build to continue
         }
         default:;  // fallthrough
     }
     return serve_target_result_t{
-        std::in_place_index<1>,
+        std::in_place_index<2>,
         fmt::format("Serve endpoint failed with:\n{}", status.error_message())};
 }
 
diff --git a/src/buildtool/serve_api/remote/target_client.hpp b/src/buildtool/serve_api/remote/target_client.hpp
index 948e9337..0d34ea2d 100644
--- a/src/buildtool/serve_api/remote/target_client.hpp
+++ b/src/buildtool/serve_api/remote/target_client.hpp
@@ -34,12 +34,17 @@
 
 /// \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.
+/// analysis/build failure received from the endpoint, as a string; this should
+/// also trigger a local build fail.
+/// Index 1 will contain the message of any INTERNAL error on the endpoint, as
+/// a string; this should trigger a local build fail.
+/// Index 2 will contain any other failure message, as a string; local builds
+/// might be able to continue, but with a warning.
+/// Index 3 will contain the target cache value information on success.
 using serve_target_result_t =
     std::variant<std::string,
                  std::string,
+                 std::string,
                  std::pair<TargetCacheEntry, Artifact::ObjectInfo>>;
 
 /// Implements client side for Target service defined in: