From ea9ffa0ebfe71fa7275746971b47ed3cee936ffa Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:41:07 +0100 Subject: [PATCH 1/5] fix(telemetry): drop the unused include from MetricsRegistry.h The header was added alongside sanitiseHandler(), which calls std::ranges::all_of -- but that algorithm lives in , which the same commit also added and which is still needed. itself is never used, so misc-include-cleaner rejects it and the clang-tidy job fails on MetricsRegistry.h:146. --- src/xrpld/telemetry/MetricsRegistry.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/xrpld/telemetry/MetricsRegistry.h b/src/xrpld/telemetry/MetricsRegistry.h index 6c10cdafc6..22d7fe2eba 100644 --- a/src/xrpld/telemetry/MetricsRegistry.h +++ b/src/xrpld/telemetry/MetricsRegistry.h @@ -143,7 +143,6 @@ #include #include #include -#include #include #include From 5e7c1bd02e70e9dd418bdb6af3db6f8ba5e35670 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:41:17 +0100 Subject: [PATCH 2/5] test(overlay): use designated initializers in the fee-case table The FeeCase table used positional aggregate initialisation, which modernize-use-designated-initializers rejects for a five-field aggregate; the clang-tidy job failed once per row. Naming the fields also makes the requested/found/derived/literal ordering readable at each row instead of relying on the reader tracking four ints positionally. --- src/test/overlay/TMGetObjectByHash_test.cpp | 76 ++++++++++++++------- 1 file changed, 50 insertions(+), 26 deletions(-) diff --git a/src/test/overlay/TMGetObjectByHash_test.cpp b/src/test/overlay/TMGetObjectByHash_test.cpp index fe058d630d..e729ffe00e 100644 --- a/src/test/overlay/TMGetObjectByHash_test.cpp +++ b/src/test/overlay/TMGetObjectByHash_test.cpp @@ -794,49 +794,73 @@ class TMGetObjectByHash_test : public beast::unit_test::Suite return { // Wholly free: at or below the free allowance nothing is // billable, so only the small size band applies -- which is 0. - {k.free, k.free, k.bandSmall, 0, "at the free allowance"}, - {0, 0, k.bandSmall, 0, "empty request"}, - {1, 1, k.bandSmall, 0, "one object, one hit"}, + {.requested = k.free, + .found = k.free, + .derived = k.bandSmall, + .literal = 0, + .why = "at the free allowance"}, + {.requested = 0, + .found = 0, + .derived = k.bandSmall, + .literal = 0, + .why = "empty request"}, + {.requested = 1, + .found = 1, + .derived = k.bandSmall, + .literal = 0, + .why = "one object, one hit"}, // All hits, one object past the allowance: one billable hit. - {k.free + 1, k.free + 1, k.hit + k.bandSmall, 1, "one billable hit"}, + {.requested = k.free + 1, + .found = k.free + 1, + .derived = k.hit + k.bandSmall, + .literal = 1, + .why = "one billable hit"}, // All misses, one past the allowance: misses are billed first, // so the single billable object is priced as a miss, not a hit. - {k.free + 1, 0, k.miss + k.bandSmall, 8, "one billable miss"}, + {.requested = k.free + 1, + .found = 0, + .derived = k.miss + k.bandSmall, + .literal = 8, + .why = "one billable miss"}, // Mixed at the small-band edge: 64 requested, 32 found. // Billable is 64-16 = 48; misses are 32 and all billable, // leaving 16 billable hits. - {k.smallMax, - 32, - (16 * k.hit) + (32 * k.miss) + k.bandSmall, - 272, - "small-band edge, mixed"}, + {.requested = k.smallMax, + .found = 32, + .derived = (16 * k.hit) + (32 * k.miss) + k.bandSmall, + .literal = 272, + .why = "small-band edge, mixed"}, // One past the small band moves to the medium surcharge. - {k.smallMax + 1, - k.smallMax + 1, - ((k.smallMax + 1 - k.free) * k.hit) + k.bandMedium, - 149, - "first medium-band size"}, + {.requested = k.smallMax + 1, + .found = k.smallMax + 1, + .derived = ((k.smallMax + 1 - k.free) * k.hit) + k.bandMedium, + .literal = 149, + .why = "first medium-band size"}, // The medium band's last size, then one past it, which moves to // the large surcharge. - {k.mediumMax, - k.mediumMax, - ((k.mediumMax - k.free) * k.hit) + k.bandMedium, - 1108, - "last medium-band size"}, - {k.mediumMax + 1, - k.mediumMax + 1, - ((k.mediumMax + 1 - k.free) * k.hit) + k.bandLarge, - 2009, - "first large-band size"}, + {.requested = k.mediumMax, + .found = k.mediumMax, + .derived = ((k.mediumMax - k.free) * k.hit) + k.bandMedium, + .literal = 1108, + .why = "last medium-band size"}, + {.requested = k.mediumMax + 1, + .found = k.mediumMax + 1, + .derived = ((k.mediumMax + 1 - k.free) * k.hit) + k.bandLarge, + .literal = 2009, + .why = "first large-band size"}, // Clamp: found > requested cannot make the miss count negative, // so the fee is the same as the all-hit case (1). - {k.free + 1, k.free + 10, k.hit + k.bandSmall, 1, "found exceeds requested"}, + {.requested = k.free + 1, + .found = k.free + 10, + .derived = k.hit + k.bandSmall, + .literal = 1, + .why = "found exceeds requested"}, }; } From e271aa2ea71d3d67b0ef1379992452da1214859e Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:41:57 +0100 Subject: [PATCH 3/5] fix(tests): clear three clang-tidy findings in the nodestore metric tests All three were rejected by clang-tidy as errors: - readability-math-missing-parentheses on 2 * kNumStored + kNumMissing - misc-const-correctness on the never-mutated AcquireStats 'quiet' - bugprone-unchecked-optional-access on the read_threads_running dereferences: branching on BEAST_EXPECT's return value hides the has_value() check from the analyser, so the guard is now a plain if and the macro asserts separately. --- src/test/nodestore/DatabaseConfig_test.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/test/nodestore/DatabaseConfig_test.cpp b/src/test/nodestore/DatabaseConfig_test.cpp index b8392daaa8..46329fdc3e 100644 --- a/src/test/nodestore/DatabaseConfig_test.cpp +++ b/src/test/nodestore/DatabaseConfig_test.cpp @@ -1076,7 +1076,8 @@ public: // Exact counts, so the denominators below are pinned independently. BEAST_EXPECT(busy.value("node_writes") == std::int64_t{kNumStored}); - BEAST_EXPECT(busy.value("node_reads_total") == std::int64_t{2 * kNumStored + kNumMissing}); + BEAST_EXPECT( + busy.value("node_reads_total") == std::int64_t{(2 * kNumStored) + kNumMissing}); BEAST_EXPECT(busy.value("node_reads_hit") == std::int64_t{2 * kNumStored}); // The two denominators differ, which is what makes the cross-checks // below able to catch a swapped accessor pair. @@ -1208,7 +1209,7 @@ public: // A quiet node publishes all seven at zero. Unlike a mean, zero is the // meaningful "no such event yet" reading for a counter, so omitting // these would lose the ability to see that nothing happened. - AcquireStats quiet; + AcquireStats const quiet; MetricSink fresh; telemetry::MetricsRegistry::observeAcquireStats(quiet, fresh.fn()); BEAST_EXPECT(fresh.names() == kLabels); @@ -1284,7 +1285,11 @@ public: // above the total. Compared as unwrapped values, since comparing two // optionals would also pass with both absent. auto const running = sink.value("read_threads_running"); - if (BEAST_EXPECT(running.has_value())) + BEAST_EXPECT(running.has_value()); + // Plain `if` rather than branching on BEAST_EXPECT's result: the + // macro's return value hides the check from static analysis, which + // then reads the dereferences below as unguarded. + if (running.has_value()) { BEAST_EXPECT(*running >= 0); BEAST_EXPECT(*running <= 3); From 6aa59629de666ad750bbff23c9d9b4494925e663 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:43:59 +0100 Subject: [PATCH 4/5] fix(tests): make the fetch-duration assertions machine-independent BEAST_EXPECT(getFetchDurationUs() > 0) failed on the macOS arm64 runner at DatabaseConfig_test.cpp:879. It is an assertion about the host, not the code: Database::fetchNodeObject() truncates each fetch to whole microseconds, and a read served from NuDB's in-memory buckets can genuinely measure under one microsecond. On a host fast enough for every read to truncate, the total legitimately stays at zero. Both fetch-duration checks now compare the accumulator against the total the scheduler's fetch reports carried, via a CountingScheduler. Each fetch is measured once and that one value feeds both, so the equality holds at any host speed while being strictly stronger than '> 0': a dropped fetch, a double-count, or the write member being returned all break it. The same reasoning is already recorded in the GTest at src/tests/libxrpl/nodestore/Database.cpp. The store-duration '> 0' checks are left alone: those writes reach disk and were not what failed. --- src/test/nodestore/DatabaseConfig_test.cpp | 75 ++++++++++++++++++++-- 1 file changed, 71 insertions(+), 4 deletions(-) diff --git a/src/test/nodestore/DatabaseConfig_test.cpp b/src/test/nodestore/DatabaseConfig_test.cpp index 46329fdc3e..25cc43f4da 100644 --- a/src/test/nodestore/DatabaseConfig_test.cpp +++ b/src/test/nodestore/DatabaseConfig_test.cpp @@ -34,6 +34,7 @@ #include #include +#include #include #include #include @@ -123,6 +124,72 @@ private: backend.store(object); } + /** + * A DummyScheduler that also totals what the fetch reports carried. + * + * Database::fetchNodeObject() measures each fetch once and uses that one + * value for both its internal accumulator and the report it hands the + * scheduler, so the report total is an independent reading of the same + * measurement. Comparing the two pins the accumulator without asserting + * that a fetch took any particular amount of time -- which is a statement + * about the machine, not the code, and fails on a host fast enough to + * serve every read in under a microsecond. + * + * @note Counters are atomic because Scheduler is called from the + * nodestore's read threads in production. These tests fetch + * synchronously on one thread, so the totals are still exact. + */ + struct CountingScheduler : DummyScheduler + { + /** + * Sum of the durations carried by every fetch report received. + */ + std::atomic reportedFetchUs{0}; + + /** + * Fetch reports received, however long each one measured. + */ + std::atomic fetchReports{0}; + + void + onFetch(FetchReport const& report) override + { + reportedFetchUs += static_cast( + std::chrono::duration_cast(report.elapsed).count()); + ++fetchReports; + } + }; + + /** + * Assert the read accumulator holds exactly what the fetch reports carried. + * + * Deliberately not `getFetchDurationUs() > 0`: an individual read served + * from NuDB's in-memory buckets can genuinely measure under one + * microsecond and truncate to zero, so on a fast enough host every read + * truncates and the total stays at zero with nothing wrong. That makes + * `> 0` an assertion about the machine rather than about the code, and it + * is what failed on the macOS runner. + * + * The equality below is machine-independent and strictly stronger: each + * fetch is measured once and that one value feeds both the accumulator + * and the report, so an accumulator that dropped a fetch, double-counted + * one, or reported the write member instead would break the equality + * however fast the host is. + * + * @param db Database whose read accumulator is checked. + * @param scheduler Scheduler that received the reports for @p db. + * @param expectedReports Fetches that must have been reported. + */ + void + expectFetchDurationMatchesReports( + Database const& db, + CountingScheduler const& scheduler, + std::uint64_t expectedReports) + { + BEAST_EXPECT(scheduler.fetchReports.load() == expectedReports); + BEAST_EXPECT(db.getFetchDurationUs() == scheduler.reportedFetchUs.load()); + } + public: void testConfig() @@ -705,7 +772,7 @@ public: { testcase("Fetch and store duration accessors"); - DummyScheduler scheduler; + CountingScheduler scheduler; beast::TempDir const nodeDb; Section nodeParams; @@ -762,7 +829,7 @@ public: BEAST_EXPECT(db->getFetchTotalCount() == kNumStored); BEAST_EXPECT(db->getFetchHitCount() == kNumStored); - BEAST_EXPECT(db->getFetchDurationUs() > 0); + expectFetchDurationMatchesReports(*db, scheduler, kNumStored); // Reads must leave the write accumulator alone. This also proves the // write accessor does not report the read member. @@ -838,7 +905,7 @@ public: { testcase("Rotating store duration accessors"); - DummyScheduler scheduler; + CountingScheduler scheduler; beast::TempDir const writableDir; beast::TempDir const archiveDir; @@ -876,7 +943,7 @@ public: BEAST_EXPECT(db.getFetchTotalCount() == kNumStored); BEAST_EXPECT(db.getFetchHitCount() == kNumStored); - BEAST_EXPECT(db.getFetchDurationUs() > 0); + expectFetchDurationMatchesReports(db, scheduler, kNumStored); BEAST_EXPECT(db.getStoreCount() == kNumStored); BEAST_EXPECT(db.getStoreDurationUs() == storeDurationAfterWrites); From cfa2fd06ca9c10d39980ee4643791ec50b8b2f1c Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:44:15 +0100 Subject: [PATCH 5/5] fix(ledger): count acquisitions satisfied from the local store recordCompletion() was reached only from done(), but init() can satisfy an acquisition entirely from the local store and return without ever calling done(). Those completions went uncounted, so acquire_completions read zero for a whole class of outcome. On a live clean-database sync it read 0 for 510 seconds while the node reached full, which was misread as the node completing nothing. Counted in init() rather than by calling done(), because done() drives the state machine: it stores the ledger, dispatches AcqDone, updates the fetch-rate average, and sets signaled_, which would disarm a later genuine done(). A counter must not change behaviour. A completionCounted_ latch, separate from signaled_, makes the count idempotent across the two independent exits so a completion is counted exactly once however it was reached. Co-Authored-By: Claude Opus 5 (1M context) --- src/xrpld/app/ledger/InboundLedger.h | 25 +++++++++++++ src/xrpld/app/ledger/detail/InboundLedger.cpp | 37 ++++++++++++++++--- 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/src/xrpld/app/ledger/InboundLedger.h b/src/xrpld/app/ledger/InboundLedger.h index c0ea5907c6..0cac2719a6 100644 --- a/src/xrpld/app/ledger/InboundLedger.h +++ b/src/xrpld/app/ledger/InboundLedger.h @@ -143,6 +143,21 @@ private: void done(); + /** + * Count this acquisition as completed, at most once. + * + * done() is not the only place an acquisition finishes: init() can satisfy + * one entirely from the local store and return without ever reaching + * done(). Both call this, and the completionCounted_ latch makes the + * second call a no-op, so a completion is counted exactly once however it + * was reached. + * + * Does nothing unless the acquisition actually succeeded, so a failed or + * still-running acquisition is never counted. + */ + void + recordCompletionOnce(); + void onTimer(bool progress, ScopedLockType& peerSetLock) override; @@ -181,6 +196,16 @@ private: bool haveState_{false}; bool haveTransactions_{false}; bool signaled_{false}; + /** + * Whether this acquisition has already been counted as completed. + * + * Separate from signaled_ because the two guard different things: + * signaled_ makes the done() state machine idempotent, while this makes + * the counter idempotent across the two independent exits that finish an + * acquisition (done() and init()'s local-hit path). Always accessed under + * mtx_. + */ + bool completionCounted_{false}; bool byHash_{true}; std::uint32_t seq_; Reason const reason_; diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index b03135e818..0f01b22b7b 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -148,6 +148,14 @@ InboundLedger::init(ScopedLockType& collectionLock) "xrpl::InboundLedger::init : valid ledger fees"); ledger_->setImmutable(); + // The local store satisfied the whole acquisition, so this is a genuine + // completion. It is counted here rather than by calling done(), because + // done() drives the state machine (it stores the ledger, dispatches + // AcqDone, and would double-store on the paths below) and a counter must + // not change behaviour. recordCompletionOnce() is idempotent, so if + // done() is later reached for this same object the count stays at one. + recordCompletionOnce(); + if (reason_ == Reason::HISTORY) return; @@ -449,6 +457,25 @@ InboundLedger::pmDowncast() return shared_from_this(); } +void +InboundLedger::recordCompletionOnce() +{ + // A failed or still-running acquisition is not a completion. Checked here + // rather than at each caller so both exits share one definition of + // success. + if (!complete_ || failed_) + return; + + // The latch, not the counter, is what makes this idempotent: the two + // exits that finish an acquisition are independent, and either can run + // first. + if (completionCounted_) + return; + + completionCounted_ = true; + app_.getAcquireStats().recordCompletion(); +} + void InboundLedger::done() { @@ -458,12 +485,12 @@ InboundLedger::done() signaled_ = true; touch(); - // Counted here rather than at any single caller because done() is the one - // funnel every peer-driven outcome passes through, and the signaled_ guard - // above makes it run at most once per acquisition. failed_ outcomes are + // done() is the funnel every peer-driven outcome passes through, but not + // every outcome: init() can satisfy an acquisition from the local store + // and return without reaching here. Both call the same idempotent helper + // so each completion is counted exactly once. failed_ outcomes are // excluded; the give-up path counts those itself. - if (complete_ && !failed_) - app_.getAcquireStats().recordCompletion(); + recordCompletionOnce(); // Finalize the acquire span with the outcome, timeout count, and peer // count. Keep it active as the ambient context across the finalize and