From fad34e852d0d11d76649d104545db1cf12843d03 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 15 Jun 2026 21:28:23 -0400 Subject: [PATCH] More AI review feedback: - Move a log message into the same block as the thing it's logging. - Change default recovery time from 1s to 2s. This may need some more tuning. - Rename the online_delete test config helper function to onlineDelete. - Copy some class values while under lock, even though they can't change, as defense in depth. - Use RAII scope unlock in healthWait(). --- cfg/xrpld-example.cfg | 2 +- src/test/app/LedgerMaster_test.cpp | 2 +- src/test/app/SHAMapStore_test.cpp | 3 +-- src/test/jtx/envconfig.h | 2 +- src/test/jtx/impl/envconfig.cpp | 2 +- src/xrpld/app/misc/SHAMapStoreImp.cpp | 22 +++++++++------------- src/xrpld/app/misc/SHAMapStoreImp.h | 2 +- 7 files changed, 15 insertions(+), 20 deletions(-) diff --git a/cfg/xrpld-example.cfg b/cfg/xrpld-example.cfg index 00a9122f75..370195f3d0 100644 --- a/cfg/xrpld-example.cfg +++ b/cfg/xrpld-example.cfg @@ -1063,7 +1063,7 @@ # recent ledgers are available. If not, then continue # sleeping for this number of seconds and # checking until healthy. -# Default is 1. +# Default is 2. # # Notes: # The 'node_db' entry configures the primary, persistent storage. diff --git a/src/test/app/LedgerMaster_test.cpp b/src/test/app/LedgerMaster_test.cpp index adc6344aaa..da91206966 100644 --- a/src/test/app/LedgerMaster_test.cpp +++ b/src/test/app/LedgerMaster_test.cpp @@ -129,7 +129,7 @@ class LedgerMaster_test : public beast::unit_test::Suite auto const deleteInterval = 8; Env env{*this, envconfig([](auto cfg) { - return online_delete(std::move(cfg), deleteInterval); + return onlineDelete(std::move(cfg), deleteInterval); })}; auto const alice = Account("alice"); diff --git a/src/test/app/SHAMapStore_test.cpp b/src/test/app/SHAMapStore_test.cpp index 3138628829..a814068aef 100644 --- a/src/test/app/SHAMapStore_test.cpp +++ b/src/test/app/SHAMapStore_test.cpp @@ -47,8 +47,7 @@ class SHAMapStore_test : public beast::unit_test::Suite static auto onlineDelete(std::unique_ptr cfg) { - using namespace jtx; - return online_delete(std::move(cfg), kDeleteInterval); + return jtx::onlineDelete(std::move(cfg), kDeleteInterval); } static auto diff --git a/src/test/jtx/envconfig.h b/src/test/jtx/envconfig.h index d0e5abdf6f..f1ee066b12 100644 --- a/src/test/jtx/envconfig.h +++ b/src/test/jtx/envconfig.h @@ -60,7 +60,7 @@ envconfig(F&& modfunc, Args&&... args) /// /// @return unique_ptr to Config instance std::unique_ptr -online_delete(std::unique_ptr cfg, std::uint32_t deleteInterval = 8); +onlineDelete(std::unique_ptr cfg, std::uint32_t deleteInterval = 8); /// @brief adjust config so no admin ports are enabled /// diff --git a/src/test/jtx/impl/envconfig.cpp b/src/test/jtx/impl/envconfig.cpp index 47db568ead..14690058ec 100644 --- a/src/test/jtx/impl/envconfig.cpp +++ b/src/test/jtx/impl/envconfig.cpp @@ -63,7 +63,7 @@ setupConfigForUnitTests(Config& cfg) namespace jtx { std::unique_ptr -online_delete(std::unique_ptr cfg, std::uint32_t deleteInterval) +onlineDelete(std::unique_ptr cfg, std::uint32_t deleteInterval) { cfg->ledgerHistory = deleteInterval; auto& section = cfg->section(Sections::kNodeDatabase); diff --git a/src/xrpld/app/misc/SHAMapStoreImp.cpp b/src/xrpld/app/misc/SHAMapStoreImp.cpp index 43ef446125..1b3daa22cd 100644 --- a/src/xrpld/app/misc/SHAMapStoreImp.cpp +++ b/src/xrpld/app/misc/SHAMapStoreImp.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -317,9 +318,8 @@ SHAMapStoreImp::run() bool const readyToRotate = validatedSeq >= lastRotated + deleteInterval_ && canDelete_ >= lastRotated - 1 && healthWait() == HealthResult::KeepGoing; - JLOG(journal_.debug()) << "run: Setting lastGoodValidatedLedger_ to " << validatedSeq; - { + JLOG(journal_.debug()) << "run: Setting lastGoodValidatedLedger_ to " << validatedSeq; // Note that this is set after the healthWait() check, so that we // don't start the rotation until the validated ledger is fully // processed. It is not guaranteed to be done at this point. It also @@ -647,37 +647,33 @@ SHAMapStoreImp::healthWait() OperatingMode mode = netOPs_->getOperatingMode(); std::unique_lock lock(mutex_); + auto const waitTime = recoveryWaitTime_; + auto const ageThreshold = ageThreshold_; auto numMissing = lastGoodValidatedLedger_ == 0 ? 0 : ledgerMaster_->missingFromCompleteLedgerRange(lastGoodValidatedLedger_, index); - while (!stop_ && (mode != OperatingMode::FULL || age > ageThreshold_ || numMissing > 0)) + while (!stop_ && (mode != OperatingMode::FULL || age > ageThreshold || numMissing > 0)) { // this value shouldn't change, so grab it while we have the // lock auto const lowerBound = lastGoodValidatedLedger_; - lock.unlock(); + ScopeUnlock unlock(lock); auto const stream = - mode != OperatingMode::FULL || age > ageThreshold_ ? journal_.warn() : journal_.info(); - JLOG(stream) << "Waiting " << recoveryWaitTime_.count() - << "s for node to stabilize. state: " + mode != OperatingMode::FULL || age > ageThreshold ? journal_.warn() : journal_.info(); + JLOG(stream) << "Waiting " << waitTime.count() << "s for node to stabilize. state: " << app_.getOPs().strOperatingMode(mode, false) << ". age " << age.count() << "s. Missing ledgers: " << numMissing << ". Expect: " << lowerBound << "-" << index << ". Complete ledgers: " << ledgerMaster_->getCompleteLedgers(); - std::this_thread::sleep_for(recoveryWaitTime_); + std::this_thread::sleep_for(waitTime); index = ledgerMaster_->getValidLedgerIndex(); age = ledgerMaster_->getValidatedLedgerAge(); mode = netOPs_->getOperatingMode(); numMissing = lowerBound == 0 ? 0 : ledgerMaster_->missingFromCompleteLedgerRange(lowerBound, index); - - lock.lock(); } - JLOG(journal_.debug()) << "healthWait: Setting lastGoodValidatedLedger_ to " << index; - lastGoodValidatedLedger_ = index; - return stop_ ? HealthResult::Stopping : HealthResult::KeepGoing; } diff --git a/src/xrpld/app/misc/SHAMapStoreImp.h b/src/xrpld/app/misc/SHAMapStoreImp.h index adcd605c23..58fd1a7132 100644 --- a/src/xrpld/app/misc/SHAMapStoreImp.h +++ b/src/xrpld/app/misc/SHAMapStoreImp.h @@ -94,7 +94,7 @@ private: /// available during an online_delete healthWait() call, sleep /// the thread for this time, and continue checking until recovery. /// See also: "recovery_wait_seconds" in xrpld-example.cfg - std::chrono::seconds recoveryWaitTime_{1}; + std::chrono::seconds recoveryWaitTime_{2}; // these do not exist upon SHAMapStore creation, but do exist // as of run() or before