Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics

# Conflicts:
#	src/xrpld/app/ledger/detail/InboundLedger.cpp

.github/workflows/telemetry-validation.yml

@@ -11,11 +11,13 @@
 # services, runs a multi-node cluster) — it validates the full telemetry
 # stack end-to-end rather than individual unit tests.
 #
-# Architecture: two jobs to leverage cached dependencies:
+# Architecture: three jobs to leverage cached dependencies:
+#   0. linux-image-tag — reads the CI image tag from the build matrix so this
+#      workflow cannot drift onto a different compiler than the main CI.
 #   1. build-xrpld — runs on a self-hosted runner inside the same container
-#      image the main CI uses (debian-bookworm-gcc-13). This ensures Conan
-#      packages are fetched from the XRPLF remote instead of built from
-#      source, and ccache hits the remote cache.
+#      image the main CI uses. This ensures Conan packages are fetched from
+#      the XRPLF remote instead of built from source, and ccache hits the
+#      remote cache.
 #   2. validate-telemetry — runs on ubuntu-latest (which has Docker) to
 #      launch the telemetry stack (OTel collector, Prometheus, Tempo, etc.)
 #      and validate the full pipeline end-to-end.
@@ -70,13 +72,33 @@ env:
   BUILD_DIR: build
 
 jobs:
+  # ── Job 0: Resolve the CI image tag ────────────────────────────────
+  # The tag is pinned once, alongside the build matrix, in linux.json. Reading
+  # it here rather than hardcoding a second copy means this workflow always
+  # builds in the same image (and therefore the same compiler) as the main CI.
+  # A hardcoded copy silently went stale and left this job on gcc 13 after the
+  # rest of CI moved to gcc 15, which broke the build on code the main CI
+  # compiled fine.
+  linux-image-tag:
+    runs-on: ubuntu-latest
+    outputs:
+      tag: ${{ steps.tag.outputs.tag }}
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
+
+      - name: Read nix image tag
+        id: tag
+        run: echo "tag=$(jq -r .image_tag .github/scripts/strategy-matrix/linux.json)" >>"${GITHUB_OUTPUT}"
+
   # ── Job 1: Build xrpld in the same container the main CI uses ──────
   # This ensures Conan binary packages are fetched from the XRPLF remote
   # (matching package IDs) and ccache hits the remote compilation cache.
   build-xrpld:
     name: Build xrpld
+    needs: linux-image-tag
     runs-on: [self-hosted, Linux, X64, heavy]
-    container: ghcr.io/xrplf/ci/debian-bookworm:gcc-13-sha-ab4d1f0
+    container: ghcr.io/xrplf/xrpld/nix-debian:${{ needs.linux-image-tag.outputs.tag }}
     timeout-minutes: 60
     env:
       CCACHE_NAMESPACE: telemetry-validation
@@ -88,7 +110,7 @@ jobs:
         uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
 
       - name: Prepare runner
-        uses: XRPLF/actions/prepare-runner@90f11ee655d1687824fb8793db770477d52afbab
+        uses: XRPLF/actions/prepare-runner@c00c22ada3bd6bcda48fcb0d62fbbab49fec8a0f
         with:
           enable_ccache: ${{ github.repository_owner == 'XRPLF' }}
 
@@ -101,6 +123,14 @@ jobs:
         with:
           subtract: 2
 
+      # The nix image ships several toolchains, so CC/CXX must be set
+      # explicitly for Conan to detect the intended one. gcc matches the
+      # debian gcc-release config the main CI builds.
+      - name: Set compiler environment
+        uses: ./.github/actions/set-compiler-env
+        with:
+          compiler: gcc
+
       - name: Setup Conan
         uses: ./.github/actions/setup-conan
 

src/test/nodestore/DatabaseConfig_test.cpp

@@ -34,6 +34,7 @@
 #include <xrpl/rdb/DatabaseCon.h>
 
 #include <algorithm>
+#include <atomic>
 #include <chrono>
 #include <cstddef>
 #include <cstdint>
@@ -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<std::uint64_t> reportedFetchUs{0};
+
+        /**
+         * Fetch reports received, however long each one measured.
+         */
+        std::atomic<std::uint64_t> fetchReports{0};
+
+        void
+        onFetch(FetchReport const& report) override
+        {
+            reportedFetchUs += static_cast<std::uint64_t>(
+                std::chrono::duration_cast<std::chrono::microseconds>(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);
 
@@ -1076,7 +1143,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 +1276,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 +1352,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);

src/xrpld/app/ledger/InboundLedger.h

@@ -204,6 +204,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;
 
@@ -392,6 +407,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_;

src/xrpld/app/ledger/detail/InboundLedger.cpp

@@ -193,6 +193,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;
 
@@ -753,6 +761,25 @@ InboundLedger::finalizeAcquireSpan(std::optional<std::size_t> peerCount) noexcep
     acquireSpan_.reset();
 }
 
+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()
 {
@@ -766,12 +793,12 @@ InboundLedger::done()
     // counts must stop being reported. See clearMissingNodeCounts().
     clearMissingNodeCounts();
 
-    // 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();
 
     // Keep the span active as the ambient context across the outcome log so
     // that line carries the span's trace_id. The activation is non-owning;