The gcc debug-coverage job rejected an unbraced `if` whose body is a GTest
assertion: EXPECT_EQ expands to an if/else, so the outer `if` leaves an else
that could bind either way, and -Werror=dangling-else refuses it. clang does
not warn, which is why only that one job failed.
Braced the span-names case that failed, then swept every test file this
branch touches for the same shape and braced the two others found, so the
next gcc run does not fail on the next one down the list.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gcc debug-coverage job failed where clang, macos and windows passed:
gcc's -Werror=type-limits rejects comparing an unsigned expression against
zero with >=, because it is always true.
The assertion was vacuous anyway. Both operands are unsigned, so the check
proved nothing, while the comment beside it says the intent was a non-zero
microsecond figure. Now asserts strictly positive, which is what it meant.
The same job also logs a CMake LTO capability probe failing on a missing
compiler-ar and prints the code-generation guard's echo line; neither is
related to this branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All eleven were reproduced locally against the same checks before fixing.
Six unused includes, left behind when the merge unioned two sets of includes
and later edits removed their only users: algorithm, functional, numeric and
thread in the job-queue test, and cstdint in the sync-state test and the load
manager. Each verified unused by grepping for every symbol the header
provides, so none is a still-needed include being dropped.
The telemetry registry header included ranges for a std::ranges::all_of call,
but that algorithm comes from algorithm, which the header already included.
Two consteval handler-name loops became std::ranges::all_of, which reads as
the predicate it is, and the two flagged fixtures are const.
Also picks up the levelization baseline the check asked for: the consensus
span-name test adds one edge from the libxrpl tests to xrpld.consensus, which
is the exact line CI's diff requested.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The write-duration test encoded the behaviour from before the store path was
timed. It populated the source database through store(), then asserted that
database's write duration was still zero, which was only true while
importDatabase was the sole timed path. Both concrete store overrides now
time their backend write, so an ordinary store run accumulates a duration
and the assertion failed.
Turned that stale assertion into a positive one: an ordinary storeBatch must
produce a non-zero duration, which is the case a real node actually
exercises. The negative half of the test, proving the accumulation is
per-database rather than a shared global, now checks that the import leaves
the source's store COUNT unchanged, since the source's duration is
legitimately non-zero from its own writes.
80552 tests across the six affected suites pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Phase-10 independently instrumented the peer object-fetch path while this
branch instrumented fresh-node sync, so the two overlapped in three places.
Resolved by keeping each side's stronger implementation rather than shipping
both.
Per-job-type waiting/running/deferred existed twice. Phase-10's version
survives: it publishes per-type gauges from JobQueue::collect(), which
snapshots under the queue lock and publishes after releasing it, a
deliberate lock-order fix against the collector's own lock. This branch's
jobq_backlog gauge and the JobQueue::getJobTypeCounts() accessor that fed it
are removed, along with their panels, assertions and reference rows.
jobq_saturation stays: it reports the whole worker pool, which phase-10 has
no equivalent for.
The histogram view helper also existed twice with identical bodies under two
names; one survives, and the microsecond ladder is now the named array
rather than boundaries repeated inline. The job_type label was declared
twice, once as a file-local constant invisible to the naming check; both it
and handler now come from the constants header.
Two things phase-10 adds are complementary, not duplicates, and are kept as
they are: the handler label, which separates the two request kinds that both
report as the same job type, and getobject_rejected_total, which counts
malformed requests where this branch's serve_refused_total counts requests
this node declined to serve.
Also fixes two naming-check failures that pre-date this merge on phase-10.
The check derived label keys only from namespaced constants, so it could not
see the per-subsystem headers' flat k-prefixed style and rejected dashboards
querying labels the code really emits. It now reads both styles, with the
enforcement rules unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Quorum and publish (A5):
- ledger_quorum_publish gauge: the trusted-validation tally against the
quorum the candidate ledger must reach, plus the gap between the
validated and published sequences. The published sequence was never
exported, so a publish pipeline falling behind healthy validation was
invisible.
- ledger_quorum_shortfall_total: counts the pre-accept early return where
a node has peers and validators yet still declines to declare a ledger
validated. That path was log-only, and it is the difference between
accumulating toward quorum and never reaching it.
Back-fill and persistence (A6):
- nodestore_latency: read and write service time. storeDurationUs_ was
declared but never written and had no accessor, so there was no write
latency signal at all. This is the direct fingerprint of a node with an
existing database syncing slower than a fresh one, where the node cache
is cold and every tree step reaches disk. Distinct from the existing
NuDB read panels, which show volume and hit ratio rather than service
time.
- ledger_replay_fallback_total and ledger_replay_outcome_total: the replay
path silently falls back to plain acquisition on timeout or failure, so
a defeated optimisation looked like ordinary slow back-fill.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sync-critical job types run at very low concurrency limits (ledgerRequest
and ledgerData allow 3 each), so a node can stall simply because those
jobs are held back behind other work. Nothing exposed that until now:
the existing job metrics are rates and quantiles of jobs that already
moved, or a single queue-wide depth.
- jobq_backlog{metric,job_type}: instantaneous waiting, running and
deferred counts per job type. Deferred is the starvation signal and had
no exposure anywhere; it is set when a type is at its concurrency limit.
- jobq_saturation{metric}: running tasks, worker-thread count and total
waiting, so a slowdown spanning several subsystems can be attributed to
worker-pool exhaustion instead of being diagnosed once per victim.
Both read through two new const accessors on JobQueue that take the
existing mutex once and copy integers, so a single reading is internally
consistent and no per-job cost is added. The job_type label reuses the
same JobTypes name helper the existing job counters use, so the two label
sets join.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Slowness on the peer object-fetch path could be observed but not
attributed. Job duration metrics carry only `job_type`, and both
`RcvGetLedger` and `RcvGetObjByHash` report as `ledgerRequest`, so a
queue-wait spike could not be traced to a handler. Nothing measured
NodeStore cost, request size, or the differential charge.
Latency now decomposes into three additive parts, each separately
measurable:
end-to-end = queue wait + NodeStore lookup + everything else
- `handler` label on job_queued_total/_started_total/_finished_total and
job_queued_us/job_running_us. The value is sanitised: a name passes
through only if non-empty and all ASCII letters, else "other". Two job
names embed a ledger sequence, so a raw label would mint one series
per ledger; the rule bounds the domain at 43 names plus "other".
- getobject_lookup_us, _request_objects, _lookups_total{result},
_rejected_total{reason} and _charge, recorded at their call sites.
All three histograms get explicit bucket views: the SDK default stops
at 10,000, which every one of them exceeds.
- Per-job-type waiting/running/deferred gauges for the 35 non-special
job types. `deferred` is the leading indicator, since addJob never
rejects -- it defers, so backpressure otherwise shows up only as
latency after the fact.
`JobQueue::collect()` snapshots the counters under the queue lock and
publishes gauges after releasing it. Writing them while holding the lock
would invert a lock order against the collector's own lock, which the
collector's flush thread already holds when it calls this hook.
Tests assert exact values, including that the charge is priced on the
requested count rather than the capped iteration count.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signals that separate a sync that is merely slow from one that will never
finish:
- sync_acquire{missing_state_nodes_max, missing_tx_nodes_max, in_flight,
received_data_depth}: how many SHAMap nodes each in-flight acquire is
still waiting for. getMissingNodes already computed this and the callers
discarded it after a trace log. A count that stays flat means the
acquire is wedged; a shrinking count means it is progressing. Recorded
once per sweep, never inside the per-node walk, and reset when a tree
completes so a finished acquire does not read as stuck forever.
- shamap_cache_hit_rate{treenode}: hit rate of the in-memory tree-node
cache, which sits above the node store, so it is distinct from the
existing NuDB ratio. A cold cache on a fresh node sends every traversal
step to disk.
- sync_acquire_no_progress_total: timer ticks where an acquire made no
progress, previously only logged.
- sync_addnode_total{good,duplicate,invalid}: whether arriving nodes are
useful, duplicated or rejected, so wasted fetch work is visible.
- sync_acquire_source_total{local,network}: whether a ledger was served
from the local store or had to be fetched.
Adds getBad()/getDuplicate() to SHAMapAddNode and an acquireProgress()
accessor on InboundLedgers so the xrpld gauge can read these without
libxrpl depending on telemetry.
ledger_seq is deliberately not a metric label: it is unbounded. Per-ledger
identity stays on the ledger.acquire span; the metrics expose bounded
aggregates instead.
The full-below cache hit rate is not exported: KeyCache updates different
counters than getHitRate() reads, so it would always report zero. That
libxrpl bug is documented rather than papered over.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Post-review fixes for the SpanGuard scope-leak change:
- SpanGuard.h: restore the injectCurrentContextToProtobuf real-class
declaration, its #else no-op stub, and the protocol::TraceContext
forward-declaration dropped during the 3->4 union merge (compile break).
- Re-parent consensus.phase.open and consensus.establish under the round
span via a new RCLConsensus::Adaptor::roundSpanContext() accessor: the
round span is now detached, so ambient parenting no longer works; the
phase spans link explicitly to the captured round context. csf::Peer
gains a matching no-op accessor so the generic engine still compiles.
- Switch five childSpan(op::X, ctx) sites to the full consensus::span::X
constants: childSpan(name, ctx) takes the name verbatim, so the suffix-
only op:: constants emitted short, non-dotted span names.
- TestTelemetry mock: add getConsensusTraceStrategy() override (base pure
virtual introduced on this branch) so the mock is not abstract.
- validate(): rewrite the trace_context comment to drop attack-surface
framing and the PR-discussion reference.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The test Peer adaptor's getTelemetry() returns a shared function-local
NullTelemetry instance and does not read any per-peer state, so
clang-tidy's readability-convert-member-functions-to-static flags it as
an error under -warnings-as-errors. Mark it static; it has no callers
that depend on it being a non-static member.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
