From 49c2382ea5ea45966ef15140fce6ad89672e956c Mon Sep 17 00:00:00 2001 From: Paul Cristian Sarbu Date: Thu, 12 Sep 2024 17:45:14 +0200 Subject: 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. --- src/buildtool/execution_api/remote/TARGETS | 4 +- .../execution_api/remote/bazel/bazel_response.cpp | 90 +++++++++++++++------- .../execution_api/remote/bazel/bazel_response.hpp | 12 ++- 3 files changed, 76 insertions(+), 30 deletions(-) (limited to 'src/buildtool/execution_api/remote') diff --git a/src/buildtool/execution_api/remote/TARGETS b/src/buildtool/execution_api/remote/TARGETS index ae67146d..ee1de5cb 100644 --- a/src/buildtool/execution_api/remote/TARGETS +++ b/src/buildtool/execution_api/remote/TARGETS @@ -34,6 +34,7 @@ , ["src/buildtool/execution_api/bazel_msg", "bazel_msg_factory"] , ["src/buildtool/auth", "auth"] , ["src/buildtool/execution_api/common", "bytestream-common"] + , ["src/utils/cpp", "expected"] , ["src/utils/cpp", "gsl"] , ["src/buildtool/common/remote", "client_common"] , ["src/buildtool/common/remote", "port"] @@ -48,7 +49,8 @@ ] , "stage": ["src", "buildtool", "execution_api", "remote"] , "private-deps": - [ ["src/buildtool/common", "artifact_digest_factory"] + [ ["@", "fmt", "", "fmt"] + , ["src/buildtool/common", "artifact_digest_factory"] , ["src/buildtool/common", "bazel_digest_factory"] , ["src/buildtool/common", "protocol_traits"] , ["src/buildtool/file_system", "file_system_manager"] diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp index c536f5dd..468a2531 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp @@ -17,7 +17,7 @@ #include #include -#include "gsl/gsl" +#include "fmt/core.h" #include "src/buildtool/common/artifact_digest.hpp" #include "src/buildtool/common/artifact_digest_factory.hpp" #include "src/buildtool/common/bazel_digest_factory.hpp" @@ -56,20 +56,28 @@ auto BazelResponse::ReadStringBlob(bazel_re::Digest const& id) noexcept return std::string{}; } -auto BazelResponse::Artifacts() noexcept -> ArtifactInfos const& { - Populate(); - return artifacts_; +auto BazelResponse::Artifacts() noexcept + -> expected, std::string> { + if (auto error_msg = Populate()) { + return unexpected{*std::move(error_msg)}; + } + return gsl::not_null( + &artifacts_); // explicit type needed for expected } -auto BazelResponse::DirectorySymlinks() noexcept -> DirSymlinks const& { - Populate(); - return dir_symlinks_; +auto BazelResponse::DirectorySymlinks() noexcept + -> expected, std::string> { + if (auto error_msg = Populate()) { + return unexpected{*std::move(error_msg)}; + } + return gsl::not_null( + &dir_symlinks_); // explicit type needed for expected } -void BazelResponse::Populate() noexcept { +auto BazelResponse::Populate() noexcept -> std::optional { // Initialized only once lazily if (populated_) { - return; + return std::nullopt; } populated_ = true; @@ -92,7 +100,9 @@ void BazelResponse::Populate() noexcept { auto digest = ArtifactDigestFactory::FromBazel(hash_type, file.digest()); if (not digest) { - return; + return fmt::format( + "BazelResponse: failed to create artifact digest for {}", + file.path()); } try { artifacts.emplace( @@ -101,8 +111,12 @@ void BazelResponse::Populate() noexcept { .type = file.is_executable() ? ObjectType::Executable : ObjectType::File}); - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "BazelResponse: unexpected failure gathering digest for " + "{}:\n{}", + file.path(), + ex.what()); } } @@ -116,8 +130,12 @@ void BazelResponse::Populate() noexcept { ArtifactDigestFactory::HashDataAs( network_->GetHashFunction(), link.target()), .type = ObjectType::Symlink}); - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "BazelResponse: unexpected failure gathering digest for " + "{}:\n{}", + link.path(), + ex.what()); } } for (auto const& link : action_result.output_directory_symlinks()) { @@ -130,8 +148,12 @@ void BazelResponse::Populate() noexcept { network_->GetHashFunction(), link.target()), .type = ObjectType::Symlink}); dir_symlinks.emplace(link.path()); // add it to set - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "BazelResponse: unexpected failure gathering digest for " + "{}:\n{}", + link.path(), + ex.what()); } } @@ -141,7 +163,9 @@ void BazelResponse::Populate() noexcept { auto digest = ArtifactDigestFactory::FromBazel(hash_type, tree.tree_digest()); if (not digest) { - return; + return fmt::format( + "BazelResponse: failed to create artifact digest for {}", + tree.path()); } ExpectsAudit(digest->IsTree()); try { @@ -149,13 +173,17 @@ void BazelResponse::Populate() noexcept { tree.path(), Artifact::ObjectInfo{.digest = *std::move(digest), .type = ObjectType::Tree}); - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "BazelResponse: unexpected failure gathering digest for " + "{}:\n{}", + tree.path(), + ex.what()); } } artifacts_ = std::move(artifacts); dir_symlinks_ = std::move(dir_symlinks); - return; + return std::nullopt; } // obtain tree digests for output directories @@ -176,7 +204,9 @@ void BazelResponse::Populate() noexcept { auto tree = BazelMsgFactory::MessageFromString( *tree_blob.data); if (not tree) { - return; + return fmt::format( + "BazelResponse: failed to create Tree for {}", + tree_blob.digest.hash()); } // The server does not store the Directory messages it just @@ -185,22 +215,30 @@ void BazelResponse::Populate() noexcept { // have to upload them manually. auto root_digest = UploadTreeMessageDirectories(*tree); if (not root_digest) { - Logger::Log(LogLevel::Error, - "uploading Tree's Directory messages failed"); - return; + auto const error = fmt::format( + "BazelResponse: failure in uploading Tree Directory " + "message for {}", + tree_blob.digest.hash()); + Logger::Log(LogLevel::Trace, error); + return error; } artifacts.emplace( action_result.output_directories(pos).path(), Artifact::ObjectInfo{.digest = *root_digest, .type = ObjectType::Tree}); - } catch (...) { - return; + } catch (std::exception const& ex) { + return fmt::format( + "BazelResponse: unexpected failure gathering digest for " + "{}:\n{}", + tree_blob.digest.hash(), + ex.what()); } ++pos; } } artifacts_ = std::move(artifacts); dir_symlinks_ = std::move(dir_symlinks); + return std::nullopt; } auto BazelResponse::UploadTreeMessageDirectories( diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp index 6e37d344..ec3e9748 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp @@ -20,9 +20,11 @@ #include // std::move #include +#include "gsl/gsl" #include "src/buildtool/execution_api/common/execution_api.hpp" #include "src/buildtool/execution_api/remote/bazel/bazel_execution_client.hpp" #include "src/buildtool/execution_api/remote/bazel/bazel_network.hpp" +#include "src/utils/cpp/expected.hpp" class BazelAction; @@ -58,8 +60,10 @@ class BazelResponse final : public IExecutionResponse { return action_id_; } - auto Artifacts() noexcept -> ArtifactInfos const& final; - auto DirectorySymlinks() noexcept -> DirSymlinks const& final; + auto Artifacts() noexcept + -> expected, std::string> final; + auto DirectorySymlinks() noexcept + -> expected, std::string> final; private: std::string action_id_{}; @@ -84,7 +88,9 @@ class BazelResponse final : public IExecutionResponse { return id.size_bytes() != 0; } - void Populate() noexcept; + /// \brief Populates the stored data, once. + /// \returns Error message on failure, nullopt on success. + [[nodiscard]] auto Populate() noexcept -> std::optional; [[nodiscard]] auto UploadTreeMessageDirectories( bazel_re::Tree const& tree) const -> std::optional; -- cgit v1.2.3