fix: Introduce retry for transient errors from DB (#3167)

tests/unit/data/BackendInterfaceTests.cpp

@@ -1,10 +1,13 @@
+#include "data/BackendInterface.hpp"
 #include "etl/CorruptionDetector.hpp"
 #include "etl/SystemState.hpp"
 #include "util/AsioContextTestFixture.hpp"
 #include "util/MockBackendTestFixture.hpp"
 #include "util/MockPrometheus.hpp"
+#include "util/Retry.hpp"
 #include "util/TestObject.hpp"
 
+#include <boost/asio/post.hpp>
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include <xrpl/basics/Blob.h>
@@ -12,7 +15,12 @@
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/XRPAmount.h>
 
+#include <chrono>
+#include <cstddef>
+#include <exception>
 #include <optional>
+#include <stdexcept>
+#include <type_traits>
 #include <vector>
 
 using namespace data;
@@ -160,3 +168,122 @@ TEST_F(
     });
     EXPECT_FALSE(backend_->cache().isDisabled());
 }
+
+// Loader and Extractor catch std::runtime_error to decide the server must amendment-block, so
+// DatabaseError has to stay outside that hierarchy.
+TEST(BackendInterfaceRetryTest, DatabaseErrorIsNotARuntimeError)
+{
+    static_assert(std::is_base_of_v<std::exception, DatabaseError>);
+    static_assert(not std::is_base_of_v<std::runtime_error, DatabaseError>);
+
+    try {
+        throw DatabaseError{"transient"};
+    } catch (std::runtime_error const&) {
+        FAIL() << "DatabaseError must not be caught as std::runtime_error - doing so would "
+                  "amendment-block the server on a transient database error";
+    } catch (std::exception const& e) {
+        EXPECT_STREQ(e.what(), "transient");
+    }
+}
+
+TEST(BackendInterfaceRetryTest, DatabaseErrorKeepsDefaultMessage)
+{
+    EXPECT_STREQ(DatabaseError{}.what(), "Transient database error. Please retry the request");
+}
+
+TEST(BackendInterfaceRetryTest, RetryOnTimeoutBlockingRetriesUntilSuccess)
+{
+    std::size_t calls = 0;
+
+    auto const result = retryOnTimeout(
+        [&calls]() -> int {
+            if (++calls < 3)
+                throw DatabaseError{};
+            return 42;
+        },
+        util::Retry::Delays{
+            .initial = std::chrono::milliseconds{1}, .max = std::chrono::milliseconds{2}
+        }
+    );
+
+    EXPECT_EQ(result, 42);
+    EXPECT_EQ(calls, 3);
+}
+
+TEST(BackendInterfaceRetryTest, RetryOnTimeoutBlockingDoesNotSwallowOtherExceptions)
+{
+    EXPECT_THROW(
+        retryOnTimeout(
+            []() -> int { throw std::runtime_error{"permanent"}; },
+            util::Retry::Delays{
+                .initial = std::chrono::milliseconds{1}, .max = std::chrono::milliseconds{1}
+            }
+        ),
+        std::runtime_error
+    );
+}
+
+struct BackendInterfaceRetryCoroTest : SyncAsioContextTest {};
+
+TEST_F(BackendInterfaceRetryCoroTest, RetryOnTimeoutCoroRetriesUntilSuccess)
+{
+    std::size_t calls = 0;
+
+    runSpawn([&calls](auto yield) {
+        auto const result = retryOnTimeout(
+            [&calls]() -> int {
+                if (++calls < 3)
+                    throw DatabaseError{};
+                return 42;
+            },
+            yield,
+            util::Retry::Delays{
+                .initial = std::chrono::milliseconds{1}, .max = std::chrono::milliseconds{2}
+            }
+        );
+
+        EXPECT_EQ(result, 42);
+    });
+
+    EXPECT_EQ(calls, 3);
+}
+
+TEST_F(BackendInterfaceRetryCoroTest, RetryOnTimeoutCoroDoesNotSwallowOtherExceptions)
+{
+    runSpawn([](auto yield) {
+        EXPECT_THROW(
+            retryOnTimeout(
+                []() -> int { throw std::runtime_error{"permanent"}; },
+                yield,
+                util::Retry::Delays{
+                    .initial = std::chrono::milliseconds{1}, .max = std::chrono::milliseconds{1}
+                }
+            ),
+            std::runtime_error
+        );
+    });
+}
+
+TEST_F(BackendInterfaceRetryCoroTest, RetryOnTimeoutCoroDoesNotBlockItsThread)
+{
+    bool ran = false;
+
+    runSpawn([&ran, this](auto yield) {
+        boost::asio::post(ctx_, [&ran]() { ran = true; });
+
+        std::size_t calls = 0;
+        retryOnTimeout(
+            [&calls]() -> int {
+                if (++calls < 2)
+                    throw DatabaseError{};
+                return 0;
+            },
+            yield,
+            util::Retry::Delays{
+                .initial = std::chrono::milliseconds{20}, .max = std::chrono::milliseconds{20}
+            }
+        );
+
+        EXPECT_TRUE(ran);
+    });
+}

tests/unit/data/cassandra/ErrorTests.cpp

@@ -0,0 +1,43 @@
+#include "data/cassandra/Error.hpp"
+
+#include <cassandra.h>
+#include <gtest/gtest.h>
+
+#include <cstdint>
+
+using namespace data::cassandra;
+
+namespace {
+
+CassandraError
+makeError(uint32_t const code)
+{
+    return CassandraError{"some error", code};
+}
+
+}  // namespace
+
+// isInvalidQuery is load bearing: DefaultExecutionStrategy::throwErrorIfNeeded treats it as the
+// only permanent failure and retries everything else, so anything wrongly reported here would be
+// retried forever (if false) or surfaced as a fatal std::runtime_error (if true).
+TEST(BackendCassandraErrorTest, IsInvalidQueryOnlyForInvalidQuery)
+{
+    EXPECT_TRUE(makeError(CASS_ERROR_SERVER_INVALID_QUERY).isInvalidQuery());
+
+    EXPECT_FALSE(makeError(CASS_ERROR_SERVER_READ_FAILURE).isInvalidQuery());
+    EXPECT_FALSE(makeError(CASS_ERROR_SERVER_WRITE_FAILURE).isInvalidQuery());
+    EXPECT_FALSE(makeError(CASS_ERROR_SERVER_READ_TIMEOUT).isInvalidQuery());
+    EXPECT_FALSE(makeError(CASS_ERROR_SERVER_UNAVAILABLE).isInvalidQuery());
+    EXPECT_FALSE(makeError(CASS_ERROR_SERVER_SYNTAX_ERROR).isInvalidQuery());
+    EXPECT_FALSE(makeError(CASS_OK).isInvalidQuery());
+}
+
+TEST(BackendCassandraErrorTest, MessageAndCodeArePreserved)
+{
+    // throwErrorIfNeeded puts message() into the DatabaseError it throws, so that treating an
+    // unclassified error as a timeout still reports what actually failed
+    auto const err = CassandraError{"received 1 responses and 1 failures", 0x1300};
+
+    EXPECT_EQ(err.message(), "received 1 responses and 1 failures");
+    EXPECT_EQ(err.code(), 0x1300u);
+}

tests/unit/data/cassandra/ExecutionStrategyTests.cpp

@@ -19,6 +19,7 @@
 #include <memory>
 #include <optional>
 #include <stdexcept>
+#include <string>
 #include <thread>
 #include <vector>
 
@@ -154,7 +155,45 @@ TEST_F(BackendCassandraExecutionStrategyTest, ReadOneInCoroutineThrowsOnTimeoutF
 
     runSpawn([&strat](boost::asio::yield_context yield) {
         auto statement = FakeStatement{};
-        EXPECT_THROW(strat.read(yield, statement), data::DatabaseTimeout);
+        EXPECT_THROW(strat.read(yield, statement), data::DatabaseError);
+    });
+}
+
+// A CL=QUORUM read failure is CASS_ERROR_SERVER_READ_FAILURE, which used to not throw at all,
+// leaving read() spinning; Times(1) pins that down.
+TEST_F(BackendCassandraExecutionStrategyTest, ReadOneInCoroutineThrowsOnQuorumReadFailure)
+{
+    auto strat = makeStrategy();
+
+    ON_CALL(
+        handle_,
+        asyncExecute(A<FakeStatement const&>(), A<std::function<void(FakeResultOrError)>&&>())
+    )
+        .WillByDefault([](auto const&, auto&& cb) {
+            auto res = FakeResultOrError{CassandraError{
+                "received 1 responses and 1 failures", CASS_ERROR_SERVER_READ_FAILURE
+            }};
+            cb(res);  // notify that item is ready
+            return FakeFutureWithCallback{res};
+        });
+    EXPECT_CALL(
+        handle_,
+        asyncExecute(A<FakeStatement const&>(), A<std::function<void(FakeResultOrError)>&&>())
+    )
+        .Times(1);
+    EXPECT_CALL(*counters_, registerReadStartedImpl(1));
+    EXPECT_CALL(*counters_, registerReadErrorImpl(1));
+
+    runSpawn([&strat](boost::asio::yield_context yield) {
+        auto statement = FakeStatement{};
+        try {
+            strat.read(yield, statement);
+            FAIL() << "expected DatabaseError";
+        } catch (data::DatabaseError const& e) {
+            EXPECT_THAT(
+                std::string{e.what()}, testing::HasSubstr("received 1 responses and 1 failures")
+            );
+        }
     });
 }
 
@@ -246,7 +285,7 @@ TEST_F(BackendCassandraExecutionStrategyTest, ReadBatchInCoroutineThrowsOnTimeou
 
     runSpawn([&strat](boost::asio::yield_context yield) {
         auto statements = std::vector<FakeStatement>(kNumStatements);
-        EXPECT_THROW(strat.read(yield, statements), data::DatabaseTimeout);
+        EXPECT_THROW(strat.read(yield, statements), data::DatabaseError);
     });
 }
 
@@ -384,7 +423,7 @@ TEST_F(BackendCassandraExecutionStrategyTest, ReadEachInCoroutineThrowsOnFailure
 
     runSpawn([&strat](boost::asio::yield_context yield) {
         auto statements = std::vector<FakeStatement>(kNumStatements);
-        EXPECT_THROW(strat.readEach(yield, statements), data::DatabaseTimeout);
+        EXPECT_THROW(strat.readEach(yield, statements), data::DatabaseError);
     });
 }
 

tests/unit/data/cassandra/SettingsProviderTests.cpp

@@ -47,7 +47,11 @@ getParseSettingsConfig(boost::json::value val)
         {"database.cassandra.write_batch_size", ConfigValue{ConfigType::Integer}.defaultValue(20)},
         {"database.cassandra.connect_timeout", ConfigValue{ConfigType::Integer}.optional()},
         {"database.cassandra.certfile", ConfigValue{ConfigType::String}.optional()},
-        {"database.cassandra.request_timeout", ConfigValue{ConfigType::Integer}.defaultValue(0)},
+        {"database.cassandra.initial_request_retry_delay",
+         ConfigValue{ConfigType::Double}.defaultValue(0.5)},
+        {"database.cassandra.max_request_retry_delay",
+         ConfigValue{ConfigType::Double}.defaultValue(5.0)},
+        {"database.cassandra.request_timeout", ConfigValue{ConfigType::Double}.optional()},
         {"database.cassandra.secure_connect_bundle", ConfigValue{ConfigType::String}.optional()},
         {"database.cassandra.username", ConfigValue{ConfigType::String}.optional()},
         {"database.cassandra.password", ConfigValue{ConfigType::String}.optional()},
@@ -163,3 +167,72 @@ TEST_F(SettingsProviderTest, CertificateConfig)
     auto const settings = provider.getSettings();
     EXPECT_EQ(settings.certificate, "certificateData");
 }
+
+TEST_F(SettingsProviderTest, RequestTimeoutAcceptsFractionalSeconds)
+{
+    auto const cfg = getParseSettingsConfig(
+        boost::json::parse(
+            R"JSON({"database.cassandra.contact_points": "127.0.0.1",
+                "database.cassandra.request_timeout": 2.5})JSON"
+        )
+    );
+    SettingsProvider const provider{cfg.getObject("database.cassandra")};
+
+    EXPECT_EQ(provider.getSettings().requestTimeout, std::chrono::milliseconds{2500});
+}
+
+TEST_F(SettingsProviderTest, RequestTimeoutStillAcceptsWholeSeconds)
+{
+    // a JSON integer must still be accepted
+    auto const cfg = getParseSettingsConfig(
+        boost::json::parse(
+            R"JSON({"database.cassandra.contact_points": "127.0.0.1",
+                "database.cassandra.request_timeout": 3})JSON"
+        )
+    );
+    SettingsProvider const provider{cfg.getObject("database.cassandra")};
+
+    EXPECT_EQ(provider.getSettings().requestTimeout, std::chrono::milliseconds{3000});
+}
+
+TEST_F(SettingsProviderTest, RetryDelaysDefaults)
+{
+    auto const cfg = getParseSettingsConfig(
+        boost::json::parse(R"JSON({"database.cassandra.contact_points": "127.0.0.1"})JSON")
+    );
+    SettingsProvider const provider{cfg.getObject("database.cassandra")};
+
+    EXPECT_EQ(provider.getInitialRetryDelay(), std::chrono::milliseconds{500});
+    EXPECT_EQ(provider.getMaxRetryDelay(), std::chrono::milliseconds{5000});
+}
+
+TEST_F(SettingsProviderTest, RetryDelaysAreIndependentOfRequestTimeout)
+{
+    auto const cfg = getParseSettingsConfig(
+        boost::json::parse(
+            R"JSON({"database.cassandra.contact_points": "127.0.0.1",
+                "database.cassandra.request_timeout": 2.5,
+                "database.cassandra.initial_request_retry_delay": 0.25,
+                "database.cassandra.max_request_retry_delay": 1.5})JSON"
+        )
+    );
+    SettingsProvider const provider{cfg.getObject("database.cassandra")};
+
+    EXPECT_EQ(provider.getSettings().requestTimeout, std::chrono::milliseconds{2500});
+    EXPECT_EQ(provider.getInitialRetryDelay(), std::chrono::milliseconds{250});
+    EXPECT_EQ(provider.getMaxRetryDelay(), std::chrono::milliseconds{1500});
+}
+
+TEST_F(SettingsProviderTest, EqualRetryDelaysDisableBackoff)
+{
+    auto const cfg = getParseSettingsConfig(
+        boost::json::parse(
+            R"JSON({"database.cassandra.contact_points": "127.0.0.1",
+                "database.cassandra.initial_request_retry_delay": 0.5,
+                "database.cassandra.max_request_retry_delay": 0.5})JSON"
+        )
+    );
+    SettingsProvider const provider{cfg.getObject("database.cassandra")};
+
+    EXPECT_EQ(provider.getInitialRetryDelay(), provider.getMaxRetryDelay());
+}