From df4f600c4370112a27c77c03ba1dad14ed0705a0 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:58:32 +0100 Subject: [PATCH] fix(test): remove the unbounded wait from the overlapping-insert round The round built each thread's batch inside the thread, before arriving at the latch, so a throw there left the remaining threads waiting on an arrival that never came -- the test hung instead of failing. A spawn loop that ended early did the same. Batches are now built before any thread starts, so nothing between spawn and arrival can throw, and a guard counts down the shortfall for threads that were never spawned before joining the ones that were. The depth accounting having moved to insert entry, depthSamples is now the denominator of the mean depth, so it gets its own assertions: equal to insertCount once every thread has been joined, and moving with the duplicate-key round. The overlap assertions are unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- include/xrpl/nodestore/WriteStats.h | 2 +- src/tests/libxrpl/nodestore/NuDBFactory.cpp | 72 +++++++++++++++++++-- 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/include/xrpl/nodestore/WriteStats.h b/include/xrpl/nodestore/WriteStats.h index 11aafff0c9..c8f485d3d1 100644 --- a/include/xrpl/nodestore/WriteStats.h +++ b/include/xrpl/nodestore/WriteStats.h @@ -73,7 +73,7 @@ struct WriteStats /** * Summed writer depth observed at each insert. Divided by - * @ref insertCount this gives mean depth. + * @ref depthSamples this gives mean depth. */ std::uint64_t depthSum = 0; diff --git a/src/tests/libxrpl/nodestore/NuDBFactory.cpp b/src/tests/libxrpl/nodestore/NuDBFactory.cpp index 2658cd3fdf..5f9197df84 100644 --- a/src/tests/libxrpl/nodestore/NuDBFactory.cpp +++ b/src/tests/libxrpl/nodestore/NuDBFactory.cpp @@ -1,9 +1,11 @@ #include +#include #include #include #include #include #include +#include #include #include @@ -76,6 +78,24 @@ constexpr std::uint64_t kOverlapPerThread = 50; * doInsert() together rather than one after another; staggered starts are what * would let every insert run end to end and never overlap. * + * The latch is the whole point of the round, and it is also the one thing here + * that can hang the test binary rather than fail it. Two rules keep it safe: + * + * 1. Every batch is built before the first thread exists, so the only thing + * a thread does before arriving is arrive. Building a batch inside the + * thread allocates, so it can throw, and a thread that throws never + * arrives -- leaving the other seven blocked on the latch for good. + * 2. Spawning is guarded, so a thread that never starts still has its + * arrival accounted for. + * + * batches built here (may throw; no thread waiting yet) + * | + * v + * spawn 8 --> [ latch: 8 arrivals ] --> stores overlap --> join + * | ^ + * +-- spawn threw ---+ guard counts down the missing arrivals, + * then joins the threads already running + * * @param backend Backend to insert into. Must be open. * @param round Round index, mixed into the seeds so every round writes * fresh keys and no insert takes the duplicate short-circuit. @@ -83,20 +103,44 @@ constexpr std::uint64_t kOverlapPerThread = 50; void runOverlappingInsertRound(Backend& backend, int round) { + std::vector batches; + batches.reserve(kOverlapThreads); + for (auto t = 0uz; t < kOverlapThreads; ++t) + { + batches.push_back(createPredictableBatch( + kOverlapPerThread, 1000 + t + (static_cast(round) * 100'000))); + } + std::latch start(static_cast(kOverlapThreads)); std::vector threads; threads.reserve(kOverlapThreads); + + // Covers a spawn loop that ends early. The latch is built for the full set + // because a thread that has already arrived cannot be un-counted, so the + // shortfall is counted down instead of the latch being resized. Counting + // down comes before joining: a thread left waiting on the latch would never + // become joinable. + // + // Released once every thread exists, so the success path joins below rather + // than from a destructor -- join() can throw, and a destructor would turn + // that into a terminate. + ScopeExit releaseAndJoin([&] { + start.count_down(static_cast(kOverlapThreads - threads.size())); + for (auto& th : threads) + th.join(); + }); + for (auto t = 0uz; t < kOverlapThreads; ++t) { - threads.emplace_back([&backend, &start, t, round] { - auto const batch = createPredictableBatch( - kOverlapPerThread, 1000 + t + (static_cast(round) * 100'000)); + threads.emplace_back([&backend, &start, &batches, t] { start.arrive_and_wait(); - for (auto const& obj : batch) + for (auto const& obj : batches[t]) backend.store(obj); }); } + releaseAndJoin.release(); + for (auto& th : threads) th.join(); } @@ -330,6 +374,7 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert) EXPECT_EQ(initial->insertTotalUs, 0u); EXPECT_EQ(initial->insertMaxUs, 0u); EXPECT_EQ(initial->depthSum, 0u); + EXPECT_EQ(initial->depthSamples, 0u); EXPECT_EQ(initial->concurrentWriters, 0u); // Exactly 10 inserts must be counted as 10, and depthSum must be 10 @@ -352,6 +397,11 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert) FAIL() << "nudb must report write stats after inserts"; EXPECT_EQ(after->insertCount, kFirstBatch); EXPECT_EQ(after->depthSum, kFirstBatch); + // The denominator of the published mean depth. One sample per insert, so + // with the depth being 1 throughout, depthSum and depthSamples coincide + // here -- which is why this pair alone cannot tell a correct depthSum from + // a constant 1, and why the overlap test below exists. + EXPECT_EQ(after->depthSamples, kFirstBatch); EXPECT_GT(after->insertTotalUs, 0u); EXPECT_GT(after->insertMaxUs, 0u); // A maximum is never below the mean, so max * n >= sum. Catches a field @@ -376,6 +426,7 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert) FAIL() << "nudb must report write stats after a second batch"; EXPECT_EQ(cumulative->insertCount, kFirstBatch + kSecondBatch); EXPECT_EQ(cumulative->depthSum, kFirstBatch + kSecondBatch); + EXPECT_EQ(cumulative->depthSamples, kFirstBatch + kSecondBatch); EXPECT_EQ(cumulative->concurrentWriters, 0u); // A running maximum never decreases. EXPECT_GE(cumulative->insertMaxUs, after->insertMaxUs); @@ -439,6 +490,10 @@ TEST(NuDBFactory, write_stats_count_duplicate_key_inserts) // rounds that stored new data would leave these equal. EXPECT_EQ(second->insertCount, first->insertCount + kBatchSize); EXPECT_EQ(second->depthSum, first->depthSum + kBatchSize); + // Sampled at entry, so the duplicate round is sampled whether or not it + // stores anything. An implementation that sampled only new data would + // leave this at the first round's figure and skew the mean. + EXPECT_EQ(second->depthSamples, first->depthSamples + kBatchSize); // The depth returned to zero, so the key_exists early return did not // leak a writer. EXPECT_EQ(second->concurrentWriters, 0u); @@ -505,6 +560,15 @@ TEST(NuDBFactory, write_stats_measure_depth_under_real_overlap) // under contention fails this. EXPECT_EQ(stats->insertCount, completedRounds * kOverlapThreads * kOverlapPerThread); + // A depth sample is taken when an insert starts and insertCount rises when + // one finishes, so the two populations differ only while an insert is in + // flight. Every thread has been joined here, so nothing is in flight and + // they must agree exactly. That is what licenses comparing depthSum against + // insertCount below, and it is an assertion in its own right: a depthSamples + // stuck at zero makes the published mean depth vanish rather than read + // wrong, because the metric is omitted when its denominator is zero. + EXPECT_EQ(stats->depthSamples, stats->insertCount); + // THE assertion this test exists for: strictly greater, so a depthSum fed // a constant 1 (or fed nothing, or fed insertCount's own delta) cannot // satisfy it however many rounds run.