Adds SHAMapStore_test coverage for the freshen guarantee: after two
online-delete rotations delete the first archive, a retained validated
ledger must still resolve from the writable backend. The test mutates
account state across ledgers, drives two rotations, clears the tree-node
and ledger caches so the check reads the backend, then reloads a retained
ledger and asserts walkLedger finds no missing node.
Catches a regression where the rotation's copy-forward/freshen fails to
rewrite an archive-only node into the writable backend: the reload would
return null or the walk would report a missing node.
clang-tidy misc-const-correctness: the cache in
for_each_key_partition_on_empty_cache_calls_back_with_empty_batches is only
read (forEachKeyPartition is const), never inserted into, so it must be
declared const. The other fixtures insert and stay non-const.
Review follow-up on the freshen lock-hold fix:
- Drop host names, dates and one-site figures from the new comments,
harness notes and docs; explain the mechanism in general terms.
- RotationPhase stores its stage and cache labels as owned std::string,
not std::string_view: the ctor still takes views so the label
constants pass without a copy, but a member view would dangle if a
caller ever passed a temporary. freshenCache/recordFreshen take the
cache name by std::string_view (read-only, call-scoped).
- The new DatabaseRotating test called fetchNodeObject through the
derived type, whose private override hides the public base method;
call it through Database& instead. This was the dev-box build break.
- freshenCache reports the exact fetched count when a health abort cuts
it short, and stops labelling the per-partition hold 'getKeys'.
- Remove a [[maybe_unused]] that silenced no warning (the build sets
-Wno-unused-parameter and disables misc-unused-parameters).
The reference doc, span-harness notes and histogram-bucket comments
named the internal AWS dev box and dates while explaining why the
rotation phases are timed. Reword to the general mechanism (a
multi-second freeze at the copy-walk to freshen boundary on a populated
node); the specific hosts, dates and trace ids stay in the task notes.
The runbook provenance paragraph named the internal AWS dev box and a
build hash and dates. State what was measured (one mainnet node, same
host and binary, differing only in store state) without the deployment
detail, which belongs in an internal runbook, not the public repo.
clang-tidy's bugprone-unchecked-optional-access does not model GTest's
ASSERT_TRUE(x.has_value()), so it flags every deref that follows one.
The reads are guarded; mark them NOLINT, matching the same suppression
in src/tests/libxrpl/consensus/LedgerTrie.cpp. .value() does not help --
the checker treats it as an unchecked access too.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The online-delete rotation's cache freshen called TaggedCache::getKeys(),
which held the cache mutex while copying every key. On the dev box's 26
million entry tree-node cache that hold lasted 5-6 s, froze every job
that touches the cache, and dropped the RocksDB node out of sync once per
rotation: each "getKeys held the lock" warning was followed within 1-5 s
by "View of consensus changed" (5 of 5 rotations on 2026-09-15).
Copy the keys one map partition at a time instead. TaggedCache gains
forEachKeyPartition(), which holds the mutex only while one partition's
keys are copied and runs the callback with the mutex released, so the
longest hold shrinks by the partition count (8 on the dev box). The
freshen.keys rotation phase no longer exists as one step, so its span,
stage value, harness entries and docs are removed; the per-partition hold
still shows on the cache lock-hold peak gauge.
Measure what the freshen achieves, which no existing signal did.
DatabaseRotating gains duplicateCopyForwardTotal(), counting archive
copies made on duplicate fetches (the rotation's own copy walk and
freshen); copyForwardTotal() deliberately excludes those. The freshen
phase records rotation_freshen_keys_total{cache,outcome} and stamps
key_count, cache and keys_copied on its span; the copy phase stamps
nodes_copied. A warn log line per freshen reports the same numbers, and
the ledger-sync-health dashboard gets a Rotation Freshen Yield panel.
Log the "STATE->" operating-mode change at warn instead of info. It is
the only record of a mode change with an exact timestamp; the
state_changes_total counter is scraped once a minute and cannot order a
flap against a multi-second event.
Tests: five GTests for forEachKeyPartition (every key once, empty cache,
mutex free during the callback, concurrent insert, lock-hold peak), three
for duplicateCopyForwardTotal over two memory backends, one for the new
counter's series, and the new name literals.
OTelCollectorHooks.cpp named nothing from Hook.h, took ResourceMetrics
without a direct include, and left the overriding
GetAggregationTemporality() unmarked. NodeIdentity.h carried <optional>
and <string> it no longer uses. All fail CI under warnings-as-errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SecretKey deletes operator==, so EXPECT_EQ on the pair's second member
does not compile; compare the byte ranges instead. Application.cpp no
longer calls logicError(), so clang-tidy's include-cleaner rejects
<xrpl/basics/contract.h> there.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Static constants take the k prefix (readability-identifier-naming), and
SpanGuardScope.cpp no longer names anything from <opentelemetry/metrics/noop.h>
since it calls noopMeter(). Both fail CI under warnings-as-errors.
The helper's docstring also claimed NoopMeterProvider hides the base
two-argument GetMeter; it declares that overload itself, so the only
detail worth sharing is the version.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
clang-tidy's include-cleaner flags Counter.h and Gauge.h here: the test
names neither type, so the includes fail CI under warnings-as-errors.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
