Phase-10 brought in the upstream nodestore/peerfinder/consensus reorganisation
along with its own write-path telemetry, which collided with the sync-diagnostic
signals on this branch. Twelve files conflicted; every resolution keeps both
intents rather than picking a side.
The nodestore write timing was implemented twice, independently. Both sides
added getStoreDurationUs()/getFetchDurationUs() to Database and both timed the
backend call in each concrete store(). Keeping both would have added twice to
storeDurationUs_ per store while storeStats() still counted one, so the mean
write latency would have read double on every dashboard -- silently, since no
test on either side asserts an exact microsecond figure. Resolved to one
accumulator API: recordStoreDuration(), which takes a duration, clamps a
sub-microsecond sample to zero and uses a relaxed atomic add. Phase-10's
storeDurationStats() is gone and its two call sites now use the survivor, so
all three store paths -- both store() overrides and importInternal() -- add
exactly once.
SlotCensus and its pure virtual moved from src/xrpld/peerfinder/ to
include/xrpl/peerfinder/PeerfinderManager.h, following the Manager interface
upstream relocated. The xrpld header is now phase-10's makeConfig shim, and
Overlay.h, MetricMacros.cpp and the getSlotCensus() override chain point at the
new location. ConsensusSpanNames.h and peerfinder Slot.h/Config.h include paths
followed their headers into libxrpl the same way.
InboundLedger gained phase-10's AcquireStats counters next to this branch's
span activations in both the destructor abort path and done(); neither
displaces the other. nodestore_state keeps the constant-based name this branch
requires of it and phase-10's fuller description.
Upstream #7292 deleted src/test/nodestore/Database_test.cpp, which held this
branch's testDurationAccessors. Phase-10 restored the per-store half of that
coverage in DatabaseConfig_test, but nothing covered importInternal -- it writes
through storeBatch() and never through store(), so it is a third store path that
has to time itself. That half is ported to a GTest in
src/tests/libxrpl/nodestore/Database.cpp, keeping the exact zero-before and
accumulate-after assertions and the per-instance negative check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
beast::Journal has no default constructor, so holding one as a plain
member deleted the suite's own default constructor and the Beast
registration macro could not instantiate it. SuiteJournal takes the
suite, converts implicitly where a journal is expected, and routes log
output into the test report.
Also removes a leftover unused local in run(), which is a hard error
under the warnings-as-errors build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The nodestore suites moved from Beast to GTest upstream, which deleted
src/test/nodestore/TestBase.h. DatabaseConfig_test stayed on Beast and
still derived its journal and batch helpers from that base, so once both
sides met in a merge it referenced three symbols that no longer existed.
It now carries its own copies, matching the current API: node object
types are NodeObjectType::Ledger rather than the old hotLEDGER spelling.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FetchReport::elapsed was milliseconds, so every nodestore read rounded to
zero: a warm store answers in single-digit microseconds and a cold one in
low hundreds, and both became 0 ms. That difference is the whole signal
separating a cold-read stall from a healthy node, and it was being
discarded at the type. Database::fetchNodeObject now measures once and
uses that one value for both the cumulative counter and the report, so
the two can never disagree. The job-queue call still takes milliseconds
and now casts explicitly.
BatchWriteReport::elapsed stays milliseconds and is documented as such:
a batch write covers many objects and reaches the disk, so it belongs in
that range.
Also adds a sub-millisecond histogram ladder, because the existing bucket
edges start at 100 microseconds and put the entire warm range in bucket
0. It is not wired to a view yet: no sub-millisecond instrument exists to
name, so the edges wait for the instrument that records read latency.
The new test captures what the nodestore reports and asserts the reported
total equals the internal microsecond accumulator exactly, plus that at
least one report is not a whole number of milliseconds -- which a
millisecond-typed field can never satisfy on any hardware.
The nodestore namespace became xrpl::node_store when develop was merged
in, but one mock override still named the old spelling, so it did not
match the ServiceRegistry signature it overrides. The sibling mock in
TestServiceRegistry.h was already correct.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the derived read and write means, the NuDB writer depth and insert
timings, and the seven acquisition counters to the existing
nodestore_state gauge. Every value multiplexes onto that one instrument
through its `metric` label, so no new instrument is created.
Means are omitted rather than reported as zero when their denominator is
zero, so a dashboard shows a gap instead of a plausible wrong number. All
four go through one new scaledMean() helper so the guard cannot be
forgotten at a future call site; it also saturates instead of wrapping,
because a wrapped gauge reads as a healthy-looking dip. A zero total over
real samples still reports zero, since a store fast enough to truncate
every sample must not look dead.
The NuDB write-path block is skipped entirely when getWriteStats() is
nullopt, which is every backend but NuDB, so absent labels distinguish
"not measured" from "measured, and idle". Writer depth is scaled by 100
and named accordingly, because it sits just above 1.0 and an integral
gauge would truncate the whole signal away.
The gauge callback body is split into four static helpers to stay inside
the per-function line budget and to make each domain testable with a
recording sink.
Also corrects nudb_bytes, which called getStoreSize() exactly as
node_written_bytes does, so the obvious write-amplification ratio was a
constant 1.0 and the old "on-disk size" comment was wrong. No file-size
accessor exists on Backend or Database, so the value is unchanged and the
comment now states what it really is.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
clang-tidy had been skipped on this branch while CMake configure was
failing, so these findings in makeStageSpan surfaced only now:
- brace the three single-statement if bodies (readability-braces-around-statements)
- compare the pointer parameter explicitly against nullptr
(readability-implicit-bool-conversion)
- include xrpl/protocol/Protocol.h for LedgerIndex (misc-include-cleaner)
A saturated ledgerData lane makes TimeoutCounter re-arm its timer
without running the timer body, so timeouts_ never advances and the
six-timeout give-up can never fire. Acquisitions then neither finish
nor fail until the one-minute sweep destroys their partial maps, and
the work restarts. Every step of that chain was debug-log-only, so a
node at warning level could not be diagnosed after the fact.
The counters are separate on purpose: deferrals rising while timeouts
stay flat is the signature, and no single counter shows it.
Completions are recorded in done() rather than at the "Done: complete"
log line, because that line also fires for failures and misses the
checkLocal and receiveNode paths; done() is the one funnel every
outcome passes through and its signaled_ guard makes it idempotent.
AcquireStats is only forward-declared in ServiceRegistry so libxrpl
still includes nothing from xrpld. The src/ include path for the test
binary moves out of the telemetry guard, since a header-only type
under src/xrpld/ is testable in every build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The consensus headers moved into the isolated xrpl/consensus module and
took a dependency on xrpl/telemetry for the tracing span constants, but
two things were left behind:
- Four includes still pointed at the old src/xrpld/consensus/ location,
which no longer exists, so the build failed with
"fatal error: 'xrpld/consensus/ConsensusParms.h' file not found".
- xrpl.libxrpl.consensus never linked xrpl.libxrpl.telemetry. add_module
isolates each module's headers, so xrpl/telemetry/SpanNames.h was not
on the include path even once the include was repointed.
Repoint the stale includes at xrpl/consensus/, and declare the telemetry
module before consensus so consensus can link it. Regenerate ordering.txt
for the resulting edge.
NuDB serializes every insert behind one global mutex held for the whole
call, so a caller cannot see how long it waited. Record instead the
writer depth joined at and the wall time spent; with mean depth L and
mean insert time W, Little's Law gives service time W/L and queueing
W - W/L. That distinguishes a serialized write path from a saturated
disk: measured on a dev box the device sat 89 percent idle while
throughput stayed flat at 42k inserts per second.
The accounting runs from a ScopeExit guard because the insert can
allocate and therefore throw; leaking the depth would strand the gauge
above zero for the life of the process.
getWriteLoad also stops returning a hardcoded zero. It now reports
writer depth, which is bounded by the writing-thread count and so stays
far below the kMaxWriteLoadAcquire cutoff that gates history
acquisition, where returning bytes or microseconds would have silently
suppressed it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The isValidJson2 call site in RPCCall.cpp already forces instantiation
of std::all_of over json::ValueConstIterator, so a regression that
removed the iterator traits would fail the real build. A dedicated
static_assert test is redundant; remove it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
json::ValueConstIterator and ValueIterator declared difference_type,
reference and pointer but not value_type or iterator_category. Under
C++23, std::iterator_traits then classifies them as output iterators,
so std::all_of over a Value's members (isValidJson2 in RPCCall.cpp)
fails to instantiate on GCC 13/14 with:
cannot convert 'output_iterator_tag' to 'std::input_iterator_tag'
GCC 15 masks this via LWG-3798/P2609, but the perf CI image ships
GCC 13, so the source needs the traits regardless. The iterators wrap
a std::map iterator (++/-- only), so the category is bidirectional.
Add value_type + iterator_category to both iterators, include <iterator>,
and add a regression test asserting the traits and that std::all_of /
std::count_if compile and run over Value members.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
Two suspects from the 3.3.0 slowdown investigation had no signal. Both were
already computing the numbers and throwing them away, so this exposes them
rather than adding measurement.
Per-sweep heap trim. The trim runs after every cache sweep, and its cost
scales with resident heap, so it is the leading explanation for a node with
a populated database syncing slower than a fresh one. The report already
carried duration, fault deltas and reclaimed pages, but the whole
measurement sat behind a debug-journal check, so an ordinary node measured
nothing, and the call site discarded the result. The measurement now always
runs and only the log line stays gated. Records trim duration, minor faults
and reclaimed kilobytes. Measured cost of the always-on path is about six
microseconds per sweep against a trim costing milliseconds, at a cadence of
ten to a hundred and twenty seconds.
Honest limit, stated in the runbook: the fault delta spans only the trim
call, so it shows the trim itself faulting but not the faults that follow as
caches refill. The duration is the signal to correlate against sweep-job
queueing.
Rotation writes. Rotation copies archive-served reads forward and re-stores
nodes missing from both backends, both of which compete with sync I/O and
only happen on a populated online_delete database. The copy-forward count
existed but was reset by the rotation's own log line, so a metric reading it
would drop to zero on every swap; a never-reset total sits beside it now.
The re-store count was not measured at all. Rotation duration is
deliberately not recorded: the health throttle sleeps at eight points inside
the sequence and dominates exactly when the node is unhealthy, so the number
would conflate work with waiting.
Nothing added for the other two suspects. Get-object serving is already
covered by the handler label, the lookup histogram and the deferred and
saturation gauges; peer churn by the disconnect-reason counter.
Also replaces nine per-file cspell ignores with one ignoreRegExpList entry
for the telemetry macro names, and picks up the levelization baseline for the
consensus span-name test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fetchDurationUs_ had no getter, so telemetry reached it by building a JSON
object, stringifying a uint64 and parsing it back with stoll on every
collect tick. storeDurationUs_ was declared and never written or read at
all, along with the jss::node_writes_duration_us key.
Both now have accessors, both production store paths time their backend
call, and the registry reads them without the round trip. get_counts also
reports the write duration, so the RPC and the metric agree. Mean read
latency is the signal that separates a cold store from a warm one: warm
reads are single-digit microseconds, cold ones low hundreds.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both were uint32_t while every sibling counter was uint64_t. On a
multi-day node node_read_bytes read 1,481,244,491 for 1,894,924,394
reads, i.e. 0.8 bytes per read, which is impossible: the counter had
wrapped about 350 times. Read hit rate is used to tell a cold-read
stall from a write-lock ceiling, so a wrapping numerator makes that
diagnosis wrong rather than merely imprecise.
getFetchTotalCount() was already backed by a uint64_t member, so its
32-bit return type truncated a correct value at the accessor. All
three getters now return uint64_t.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by reviewing what each metric actually measures, with attention to the
derived and bucketed ones. All five could report healthy while the node was
not, or the reverse.
- The nodestore latency panel took rate() of a mean. The gauge already
divides duration by count in code, so rating it produced a figure with no
unit, and Prometheus discards a gauge's decreases, so a heavy back-fill
read as roughly zero microseconds per operation. The cumulative duration
totals are now exported alongside the means, and the panel divides the
rate of the total by the rate of the count, which is the latency over the
panel's own window rather than a since-boot average that flattens with
uptime.
- The DNS-resolve and outbound-dial histograms had no explicit buckets, so
they inherited a ladder that stops at ten seconds while the dial timer is
fifteen. Every timed-out dial fell in the overflow bucket and p95 read
exactly ten seconds however bad it got. Both now have a ladder reaching
thirty seconds with fifteen on its own boundary, so a timeout is
distinguishable from merely slow.
- The missing-node counts only cleared when a tree completed, so a
timed-out or failed acquire left its last count latched. Since the gauge
reports the maximum across everything still in the collection, and
eviction waits on a grace period plus the sweep interval, a finished node
reported as stuck for minutes. That inverts the one signal that separates
stuck from slow. Cleared unconditionally on the terminal path instead.
- A disabled quorum published a sentinel so large that, on a timeseries
axis shared with the trusted-key count, it flattened the key line to the
baseline and hid the outage it was meant to mark. The series is now
omitted and a quorum_disabled flag carries the state.
- Two panel descriptions claimed a one-second export cycle. The reader is
configured for ten.
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>
These are the first failures found by actually building and running the new
telemetry tests. All three were defects in the tests, not the metrics.
- state_changes_total expected six series from seven transitions, but two
edges were traversed twice, so there are five distinct from/to pairs. The
repeat is the point of the label: flapping raises the count on one edge
rather than minting a new series, so the assertion now says five and
explains why.
- shamap_cache_hit_rate compared for exact double equality against a value
that reaches the gauge through a float hit rate, so 0.9 arrives as
0.89999997. Now compares within a tolerance far tighter than any
threshold a dashboard reads, since asserting exact equality was only
asserting the float representation.
- nodestore_latency shared one provider across four scenarios. The reader
reports cumulative temporality, so a mean observed by an earlier scenario
was still present in the next collection, which defeated the two
assertions that a mean is absent when its denominator is zero. Each
scenario now collects from its own provider.
139 of 139 telemetry tests 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>
Metric names and label keys were bare string literals, repeated across the
emit site, the gauge registration, the unit test, the workload manifest, the
dashboard queries and the reference table. A rename touched six places and a
typo in any one of them failed silently: a metric that never appears, or a
label that never joins.
The span side already had this right, with names and attribute keys declared
once in the *SpanNames.h headers and a CI rule rejecting literals at call
sites. That rule only ever covered spans, so the metric side had no
equivalent and no suffix convention was enforced by anything.
- Adds MetricNames.h declaring every instrument name, label key and bounded
label value this story emits, grouped by subsystem, following the existing
span-name header layout.
- Converts the call sites subsystem by subsystem. The emitted strings are
unchanged: 75 names before, the same 75 after, verified by extracting the
wire strings from both trees and diffing the sets.
- Extends the naming check with three rules: no literal instrument name or
label key at an emit site, the duration and counter suffix conventions,
and every name in the workload manifest resolving to a constant. The
first rule is ratcheted per metric family so the pre-existing families
warn rather than block, keeping the remaining work visible instead of
forcing one unreviewable change.
Constants are character arrays rather than the span headers' StaticStr,
because the metrics API takes a string view that will not construct from it.
Two things the conversion exposed: a serve-refusal reason that the original
inventory missed because it is passed through a ternary, and a label whose
constant made it invisible to the checker's literal scan, which would have
failed a dashboard rule.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The board and runbook had grown by append across eight work packages, so
they read in the order the work was done rather than the order a node
progresses. This is the coherence pass; it adds no new instrumentation.
- Dashboard: 52 panels regrouped from two rows into nine that follow the
fresh-start sequence — bootstrap, peer supply, sync state, acquire and
SHAMap fetch, job queue, quorum and publish, terminal blockers, then
back-fill and spans collapsed since they answer conditional questions.
Layout only: no title, query or description changed.
- Runbook: the flat step list becomes a decision tree branching on the
observed symptom, with the amendment-block check first because it is
terminal. Each branch names the panels, what healthy and unhealthy look
like, and what to conclude. The existing steps are kept as the detail
bodies.
- Reference table: every signal name re-checked against the code and every
named panel against the board; four stale panel references fixed.
- Validation: every signal is now either asserted or covered by a note
explaining why a five-node local cluster cannot produce it.
Also fixes the write-latency signal, which was inert on a real node: the
store duration was only recorded on the database-import path, while the two
production store implementations did not time themselves, so an ordinary
node reported a write count with no latency. Both now time the backend
write, which is the disk work this signal exists to expose. Without it the
"existing database syncs slower than a fresh one" diagnosis had no primary
signal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes the join left open when the acquire and consensus work landed in
parallel. The acquire span now derives its trace id from the ledger hash it
already records, so a fetch shares one trace with that ledger's validation,
acceptance and store, and its three phase children inherit the same id.
Reading one trace now answers the whole question for a slow ledger: whether
the data was slow to arrive, slow to be accepted, or slow to persist.
The acquire span stays an optional member of the join group, for the reason
its own entry already gives: a healthy cluster agreeing from genesis rarely
back-fills, so the span need not appear on every run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A slow fresh-sync ledger produced spans scattered across threads with no
way to relate them. They now share a trace id derived from the ledger's own
hash, the one value every participating site already holds, so nothing new
is plumbed across threads. This is the pattern the transaction pipeline
already uses for its tx id.
Joined: ledger.validate, ledger.store, and a new
consensus.validation.accept recorded when a trusted validation arrives. In
Tempo, searching one ledger hash returns them together, so an operator can
tell whether the ledger was slow to arrive, slow to be accepted, or slow to
be stored. They are siblings rather than a chain because the accept gate is
entered from three different threads, so no fixed parent order exists.
consensus.validation.accept also records why an arriving validation did or
did not advance the gate, which makes "validations arrive but are all
rejected" visible for the first time.
consensus_round_duration_ms turns the existing round-time span attribute
into a histogram, so a fleet trend needs a metric query rather than raw
trace inspection. An explicit bucket view is required, not optional: the
SDK default tops out at ten seconds while consensus abandons a round at two
minutes, so slow rounds would all fall in one bucket and every quantile
would read exactly ten seconds. Cost is one record per round.
Record layer: the histogram is native and needs no collector change. The
two new bounded attributes are added as span-metric dimensions to both
collector configs. The ledger hash stays out of them, since a per-ledger
dimension mints a series per ledger; it is indexed in Tempo as the join key.
The ledger.acquire span is not joined yet, because that file was being
changed concurrently. It is registered as an optional member of the join
group so nothing fails, and switching it is a one-line follow-up.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four blind spots in the sync exchange, each now a span:
- txset.acquire: transaction-set acquisition had no span at all, though it
is the sibling of ledger.acquire and runs every consensus round. A round
that falls behind because its tx set never arrived was indistinguishable
from one that deliberated slowly.
- ledger.acquire.{header,astree,txtree}: the acquire span was flat, so the
account-state tree, which dominates a fresh sync, could not be separated
from the transaction tree. These are children, closed before the parent.
- peer.dial: the outbound dial already had outcome counters; the span adds
the per-attempt timeline, so a slow stage is visible rather than only its
terminal reason.
- ledger.serve: serving a peer's ledger request was uninstrumented, so this
node's contribution to someone else's sync was invisible.
Every span finalizes exactly once. Outcomes come from shared compile-time
rules rather than a literal per branch, so no exit can mislabel itself and
an exit added later cannot omit one. Destructor paths are noexcept.
One rule needed care: the timeout path also sets the failure flag, because
that is how the timeout loop stops, so precedence puts timeout ahead of
failure or a timed-out acquire would read as a data fault.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
