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

@@ -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.

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");

src/test/app/SHAMapStore_test.cpp

@@ -47,8 +47,7 @@ class SHAMapStore_test : public beast::unit_test::Suite
     static auto
     onlineDelete(std::unique_ptr<Config> cfg)
     {
-        using namespace jtx;
-        return online_delete(std::move(cfg), kDeleteInterval);
+        return jtx::onlineDelete(std::move(cfg), kDeleteInterval);
     }
 
     static auto

src/test/jtx/envconfig.h

@@ -60,7 +60,7 @@ envconfig(F&& modfunc, Args&&... args)
 ///
 /// @return unique_ptr to Config instance
 std::unique_ptr<Config>
-online_delete(std::unique_ptr<Config> cfg, std::uint32_t deleteInterval = 8);
+onlineDelete(std::unique_ptr<Config> cfg, std::uint32_t deleteInterval = 8);
 
 /// @brief adjust config so no admin ports are enabled
 ///

src/test/jtx/impl/envconfig.cpp

@@ -63,7 +63,7 @@ setupConfigForUnitTests(Config& cfg)
 namespace jtx {
 
 std::unique_ptr<Config>
-online_delete(std::unique_ptr<Config> cfg, std::uint32_t deleteInterval)
+onlineDelete(std::unique_ptr<Config> cfg, std::uint32_t deleteInterval)
 {
     cfg->ledgerHistory = deleteInterval;
     auto& section = cfg->section(Sections::kNodeDatabase);

src/xrpld/app/misc/SHAMapStoreImp.cpp

@@ -8,6 +8,7 @@
 #include <xrpl/basics/ByteUtilities.h>
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/contract.h>
+#include <xrpl/basics/scope.h>
 #include <xrpl/beast/core/CurrentThreadName.h>
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/beast/utility/instrumentation.h>
@@ -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;
 }
 

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