diff --git a/src/xrpld/app/ledger/InboundLedger.h b/src/xrpld/app/ledger/InboundLedger.h index c0ea5907c6..0cac2719a6 100644 --- a/src/xrpld/app/ledger/InboundLedger.h +++ b/src/xrpld/app/ledger/InboundLedger.h @@ -143,6 +143,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; @@ -181,6 +196,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 b03135e818..0f01b22b7b 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -148,6 +148,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; @@ -449,6 +457,25 @@ InboundLedger::pmDowncast() return shared_from_this(); } +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() { @@ -458,12 +485,12 @@ InboundLedger::done() signaled_ = true; touch(); - // 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(); // Finalize the acquire span with the outcome, timeout count, and peer // count. Keep it active as the ambient context across the finalize and