tests clean up

include/xrpl/core/JobQueue.h

@@ -29,6 +29,7 @@
 #include <map>
 #include <memory>
 #include <mutex>
+#include <optional>
 #include <set>
 #include <string>
 #include <type_traits>
@@ -237,9 +238,15 @@ public:
 
     /**
      * Block until no jobs running.
+     *
+     * Without a timeout this waits indefinitely and always returns true, which
+     * is what shutdown paths want: there is nowhere useful to report a timeout
+     * from a destructor. With one it returns false if jobs were still running
+     * when the timeout expired, so a caller that must not hang -- a test, say
+     * -- can report a failure instead.
      */
-    void
-    rendezvous();
+    bool
+    rendezvous(std::optional<std::chrono::milliseconds> const& timeout = {});
 
     void
     stop();

src/libxrpl/core/detail/JobQueue.cpp

@@ -15,6 +15,7 @@
 #include <chrono>
 #include <memory>
 #include <mutex>
+#include <optional>
 #include <set>
 #include <tuple>
 #include <utility>
@@ -239,11 +240,18 @@ JobQueue::getJson(int c)
     return ret;
 }
 
-void
-JobQueue::rendezvous()
+bool
+JobQueue::rendezvous(std::optional<std::chrono::milliseconds> const& timeout)
 {
+    auto const idle = [this] { return processCount_ == 0 && jobSet_.empty(); };
+
     std::unique_lock<std::mutex> lock(mutex_);
-    cv_.wait(lock, [this] { return processCount_ == 0 && jobSet_.empty(); });
+    if (!timeout)
+    {
+        cv_.wait(lock, idle);
+        return true;
+    }
+    return cv_.wait_for(lock, *timeout, idle);
 }
 
 JobTypeData&

src/test/app/LedgerMaster_test.cpp

@@ -3,6 +3,7 @@
 #include <test/jtx/amount.h>
 #include <test/jtx/envconfig.h>
 #include <test/jtx/noop.h>
+#include <test/jtx/shamapstore.h>
 
 #include <xrpld/app/ledger/LedgerMaster.h>
 #include <xrpld/app/misc/SHAMapStore.h>
@@ -136,15 +137,42 @@ class LedgerMaster_test : public beast::unit_test::Suite
 
         auto& lm = env.app().getLedgerMaster();
         LedgerIndex minSeq = 2;
-        LedgerIndex maxSeq = env.closed()->header().seq;
         auto& store = env.app().getSHAMapStore();
-        BEAST_EXPECT(store.rendezvous());
+        // Which of the existing complete ledgers the store initializes
+        // lastRotated from is a timing detail; all this test needs is that it is
+        // one of them. Everything below derives from the observed value rather
+        // than assuming a particular one.
+        //
+        // The range check and the initializeStore() one both end the testcase
+        // rather than merely reporting, because lastRotated is the only value
+        // from the store that enters minSeq. A lastRotated of 0 -- the value
+        // getLastRotated() reports until the store has been handed a
+        // validated ledger -- makes minSeq 0 below, and the minSeq - 1 and
+        // minSeq - 2 ranges then underflow to first > last, which aborts a
+        // Debug build inside missingFromCompleteLedgerRange().
+        auto const extraCloses = initializeStore(env);
+        if (!BEAST_EXPECT(extraCloses.has_value()))
+            return;
+        LedgerIndex maxSeq = env.closed()->header().seq;
         LedgerIndex lastRotated = store.getLastRotated();
-        BEAST_EXPECTS(maxSeq == 3, to_string(maxSeq));
-        BEAST_EXPECTS(lm.getCompleteLedgers() == "2-3", lm.getCompleteLedgers());
-        BEAST_EXPECTS(lastRotated == 3, to_string(lastRotated));
+        if (!BEAST_EXPECTS(lastRotated >= minSeq && lastRotated <= maxSeq, to_string(lastRotated)))
+            return;
+        // The BEAST_EXPECT above already returned if this is nullopt, but that
+        // is invisible to clang-tidy's optional model.
+        // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
+        BEAST_EXPECTS(maxSeq == 3 + *extraCloses, to_string(maxSeq));
+        std::stringstream initialRange;
+        initialRange << minSeq << "-" << maxSeq;
+        BEAST_EXPECTS(lm.getCompleteLedgers() == initialRange.str(), lm.getCompleteLedgers());
         BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0);
-        BEAST_EXPECT(minSeq + 1 > maxSeq - 1);
+        // The inner range is empty unless initializeStore() had to close extra
+        // ledgers, and missingFromCompleteLedgerRange() treats first > last as a
+        // precondition violation that aborts a Debug build via UNREACHABLE, so
+        // only check it when it is well formed.
+        if (minSeq + 1 <= maxSeq - 1)
+        {
+            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0);
+        }
         BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2);
         BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2);
         BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2);
@@ -157,7 +185,7 @@ class LedgerMaster_test : public beast::unit_test::Suite
                 env(noop(alice));
             }
             env.close();
-            BEAST_EXPECT(store.rendezvous());
+            BEAST_EXPECT(syncStore(env));
 
             ++maxSeq;
 
@@ -173,7 +201,19 @@ class LedgerMaster_test : public beast::unit_test::Suite
             expectedRange << minSeq << "-" << maxSeq;
             BEAST_EXPECTS(lm.getCompleteLedgers() == expectedRange.str(), lm.getCompleteLedgers());
             BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0);
-            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0);
+            // missingFromCompleteLedgerRange() treats first > last as a
+            // precondition violation and aborts a Debug build via UNREACHABLE.
+            // The range can only collapse if this test's model of minSeq /
+            // maxSeq has desynced from the store, so report that as a failure
+            // instead of taking down the whole unit test job.
+            if (minSeq + 1 <= maxSeq - 1)
+            {
+                BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0);
+            }
+            else
+            {
+                BEAST_EXPECTS(false, to_string(minSeq) + "-" + to_string(maxSeq));
+            }
             BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2);
             BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2);
             BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2);

src/test/jtx/impl/shamapstore.cpp

@@ -0,0 +1,56 @@
+#include <test/jtx/shamapstore.h>
+
+#include <test/jtx/Env.h>
+
+#include <xrpld/app/main/Application.h>
+#include <xrpld/app/misc/SHAMapStore.h>
+
+#include <xrpl/core/JobQueue.h>
+
+#include <chrono>
+#include <optional>
+#include <ostream>
+
+namespace xrpl::test::jtx {
+
+bool
+syncStore(Env& env, std::chrono::milliseconds const timeout)
+{
+    // Drain the job queue first, so that onLedgerClosed() has run and working_
+    // is set. Then wait for the store itself, so that a store which never
+    // finishes fails the caller's test instead of blocking on it.
+    //
+    // Both waits are bounded, and the same bound serves both: a caller that
+    // reaches the second one has already spent whatever the first one took, but
+    // the total is only relevant when something is broken, and then the exact
+    // total does not matter.
+    return env.app().getJobQueue().rendezvous(timeout) &&
+        env.app().getSHAMapStore().rendezvous(timeout);
+}
+
+std::optional<int>
+initializeStore(Env& env, int const maxExtraCloses)
+{
+    auto& store = env.app().getSHAMapStore();
+
+    for (int extraCloses = 0;; ++extraCloses)
+    {
+        if (!syncStore(env))
+            return std::nullopt;
+        if (store.getLastRotated() != 0 || extraCloses == maxExtraCloses)
+        {
+            if (extraCloses != 0)
+            {
+                env.test.log << "initializeStore: the store needed " << extraCloses
+                             << " extra ledger close(s) to pick up a validated ledger. "
+                                "SHAMapStoreImp::run() dropped the notification for the "
+                                "first one; see the comment on initializeStore()."
+                             << std::endl;
+            }
+            return extraCloses;
+        }
+        env.close();
+    }
+}
+
+}  // namespace xrpl::test::jtx

src/test/jtx/shamapstore.h

@@ -0,0 +1,76 @@
+#pragma once
+
+#include <test/jtx/Env.h>
+
+#include <chrono>
+#include <optional>
+
+namespace xrpl::test::jtx {
+
+/**
+ * How long the waits here allow before giving up. Long enough that a loaded CI
+ * machine does not trip it, short enough that something genuinely stuck fails
+ * the test rather than hanging the unit test job until CI's global timeout.
+ */
+inline constexpr std::chrono::seconds kRendezvousTimeout{60};
+
+/**
+ * Wait until the SHAMapStore has finished processing the ledger that the
+ * preceding env.close() produced. Returns false if either wait times out.
+ *
+ * env.close() returns as soon as the ledger_accept RPC returns, but the
+ * validated ledger path -- LedgerMaster::setValidLedger() ->
+ * SHAMapStore::onLedgerClosed() -- runs on a job queue thread. Without draining
+ * the job queue first, the store may not have been handed the ledger at all, in
+ * which case rendezvous() observes working_ == false and returns immediately,
+ * before any work has been done.
+ */
+[[nodiscard]] bool
+syncStore(Env& env, std::chrono::milliseconds timeout = kRendezvousTimeout);
+
+/**
+ * Bring the SHAMapStore to the point where it has been handed a validated
+ * ledger and initialized lastRotated, and report how many extra ledgers had to
+ * be closed to get it there (normally none). Returns std::nullopt if syncStore()
+ * itself failed.
+ *
+ * syncStore() alone does not guarantee that, because SHAMapStoreImp::run()'s
+ * loop does not use the notification and the working_ flag safely:
+ *
+ *   * onLedgerClosed() notifies cond_ whether or not run()'s thread is parked
+ *     on it, and run() waits on cond_ without a predicate, so a notification
+ *     that lands while the thread is still starting up -- before it first
+ *     reaches that wait -- is lost.
+ *   * run() clears working_ at the top of its loop without checking whether
+ *     newLedger_ is still set, so rendezvous() can report the store idle with a
+ *     validated ledger queued.
+ *
+ * Either way the store ends up parked with work pending, and only another
+ * notification gets it moving again. In a standalone test nothing else closes
+ * ledgers, so that has to come from here: this closes a ledger rather than
+ * polling getLastRotated(), because polling would just time out.
+ * onLedgerClosed() keeps only the most recent ledger in newLedger_, so the
+ * ledger the store picks up -- and therefore lastRotated -- is a timing detail,
+ * which is why callers should derive their expectations from the value they
+ * observe instead of assuming one.
+ *
+ * run() is deliberately left as it is. In production the only effect is
+ * latency: the trigger is validatedSeq >= lastRotated + deleteInterval, so a
+ * lost notification delays rotation to the next validated ledger and nothing is
+ * skipped or accumulated -- starting at 513 instead of 512 does not matter. Two
+ * consequences do follow from leaving it in place, and both hold today: nothing
+ * in production decides anything from working_ or rendezvous() (rendezvous()
+ * has no production callers at all), and a node whose ledgers only advance on
+ * demand -- standalone, driven by ledger_accept -- can sit on a queued ledger
+ * until something closes the next one, which is exactly the situation this
+ * helper is working around.
+ *
+ * So this helper is permanent rather than a stopgap. Working around the race
+ * must not make it invisible, so every extra close is logged. That keeps how
+ * often it is actually hit observable in the unit test output -- which is the
+ * only signal left once the testcases that use it stop flaking on it.
+ */
+[[nodiscard]] std::optional<int>
+initializeStore(Env& env, int maxExtraCloses = 3);
+
+}  // namespace xrpl::test::jtx