diff --git a/.github/workflows/telemetry-validation.yml b/.github/workflows/telemetry-validation.yml index e65505bf88..7fbd9021e5 100644 --- a/.github/workflows/telemetry-validation.yml +++ b/.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 diff --git a/src/test/nodestore/DatabaseConfig_test.cpp b/src/test/nodestore/DatabaseConfig_test.cpp index b8392daaa8..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); @@ -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); diff --git a/src/xrpld/app/ledger/InboundLedger.h b/src/xrpld/app/ledger/InboundLedger.h index 7543295338..9636b5571b 100644 --- a/src/xrpld/app/ledger/InboundLedger.h +++ b/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_; diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index bc9e5b8ad0..00f51ab3bc 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/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 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;