From bdfbb78115f2988a0288fd985ec44c2a58fc0f3e Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Mon, 24 Aug 2026 15:15:26 +0100 Subject: [PATCH] fix: Fiber suspension during exception (#3187) --- src/data/BackendInterface.hpp | 17 +++++++---- src/util/Retry.cpp | 11 +++++++ tests/unit/data/BackendInterfaceTests.cpp | 36 ++++++++++++++++++++++- 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/data/BackendInterface.hpp b/src/data/BackendInterface.hpp index 6e1e70f69..9f5d2ae6d 100644 --- a/src/data/BackendInterface.hpp +++ b/src/data/BackendInterface.hpp @@ -103,16 +103,21 @@ retryOnTimeout( auto retry = util::makeRetryExponentialBackoff(delays, yield.get_executor()); while (true) { + // Suspending a coroutine from inside a catch block is not safe so we copy out the failure. + std::string failure; + try { return func(); } catch (DatabaseError const& e) { - auto const delayMs = - std::chrono::duration_cast(retry.delayValue()).count(); - LOG(log.error()) << e.what() << " (attempt " << retry.attemptNumber() + 1 - << "). Retrying in " << delayMs << "ms ..."; - - retry.wait(yield); + failure = e.what(); } + + auto const delayMs = + std::chrono::duration_cast(retry.delayValue()).count(); + LOG(log.error()) << failure << " (attempt " << retry.attemptNumber() + 1 + << "). Retrying in " << delayMs << "ms ..."; + + retry.wait(yield); } } diff --git a/src/util/Retry.cpp b/src/util/Retry.cpp index d45a1f2d2..f3aa6370a 100644 --- a/src/util/Retry.cpp +++ b/src/util/Retry.cpp @@ -1,5 +1,7 @@ #include "util/Retry.hpp" +#include "util/Assert.hpp" + #include #include #include @@ -8,6 +10,7 @@ #include #include #include +#include #include #include @@ -95,6 +98,14 @@ ExponentialBackoffStrategy::nextDelay() const void Retry::wait(boost::asio::yield_context yield) { + // Suspending a coroutine while an exception is being handled corrupts the caught-exception + // state, which is per-thread rather than per-coroutine. Copy whatever is needed out of the + // handler and let it exit before waiting. + ASSERT( + std::current_exception() == nullptr, + "Retry::wait must not be called while an exception is being handled" + ); + *canceled_ = false; timer_.expires_after(strategy_->getDelay()); strategy_->increaseDelay(); diff --git a/tests/unit/data/BackendInterfaceTests.cpp b/tests/unit/data/BackendInterfaceTests.cpp index c69dd7b18..0b646f893 100644 --- a/tests/unit/data/BackendInterfaceTests.cpp +++ b/tests/unit/data/BackendInterfaceTests.cpp @@ -264,6 +264,40 @@ TEST_F(BackendInterfaceRetryCoroTest, RetryOnTimeoutCoroDoesNotSwallowOtherExcep }); } +// An exception left in flight across a suspension point is visible to whatever else runs on that +// thread meanwhile, and is corrupted when the handlers unwind out of order or on a different thread +// of the pool. We prevent this. +TEST_F(BackendInterfaceRetryCoroTest, RetryOnTimeoutCoroDoesNotWaitInsideCatchHandler) +{ + std::optional exceptionInFlightDuringWait; + + runSpawn([&exceptionInFlightDuringWait, this](auto yield) { + // Runs on this thread while the retry below is suspended on its timer. + boost::asio::post(ctx_, [&exceptionInFlightDuringWait]() { + exceptionInFlightDuringWait = std::current_exception() != nullptr; + }); + + std::size_t calls = 0; + retryOnTimeout( + [&calls]() -> int { + if (++calls < 2) + throw DatabaseError{}; + return 0; + }, + yield, + util::Retry::Delays{ + .initial = std::chrono::milliseconds{1}, .max = std::chrono::milliseconds{1} + } + ); + }); + + ASSERT_TRUE(exceptionInFlightDuringWait.has_value()) + << "the posted handler was expected to run while the retry was waiting"; + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) + EXPECT_FALSE(*exceptionInFlightDuringWait) + << "retry.wait() must not be reached from inside a catch handler"; +} + TEST_F(BackendInterfaceRetryCoroTest, RetryOnTimeoutCoroDoesNotBlockItsThread) { bool ran = false; @@ -280,7 +314,7 @@ TEST_F(BackendInterfaceRetryCoroTest, RetryOnTimeoutCoroDoesNotBlockItsThread) }, yield, util::Retry::Delays{ - .initial = std::chrono::milliseconds{20}, .max = std::chrono::milliseconds{20} + .initial = std::chrono::milliseconds{1}, .max = std::chrono::milliseconds{1} } );