From b4ee0b2f4457bfca1e021c335453dc2c20035569 Mon Sep 17 00:00:00 2001
From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
Date: Fri, 14 Aug 2026 21:44:23 +0100
Subject: [PATCH] feat(ledger): mark abandoned ledger acquisitions on the
 acquire span

An InboundLedger destroyed while !isDone() recorded recordAbort() in the
metrics but left ledger.acquire carrying only the attributes set at
construction, so an abandoned acquisition was indistinguishable from one
still in flight.

Set outcome=aborted plus timeouts on that path, and add the val::aborted
constant. peer_count is deliberately omitted: reading it goes through
Overlay, and a destructor must not depend on Overlay still existing.

The status stays Unset for an abort, because InboundLedgers::stop()
clears every in-flight acquisition, so a clean shutdown would otherwise
report errors. done() now sets Error when failed_, which is an
unambiguous failure of the operation. Success is left Unset rather than
Ok, per the OpenTelemetry guidance that instrumentation should not
assert Ok.
---
 src/xrpld/app/ledger/detail/InboundLedger.cpp | 31 +++++++++++++++++++
 src/xrpld/app/ledger/detail/LedgerSpanNames.h |  6 ++++
 2 files changed, 37 insertions(+)

diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp
index 742313f915..fbe129b37e 100644
--- a/src/xrpld/app/ledger/detail/InboundLedger.cpp
+++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp
@@ -227,6 +227,27 @@ InboundLedger::~InboundLedger()
         // the whole acquisition has to start over. That is the expensive case,
         // so it is counted apart from a cheap abort that had nothing yet.
         app_.getAcquireStats().recordAbort(haveHeader_ || haveState_ || haveTransactions_);
+
+        // Mark the span so an abandoned acquisition is distinguishable from one
+        // that was still in flight when the trace was read. Without this the
+        // span still ends (the guard's destructor calls End()) but carries only
+        // the attributes set at construction, and the collector's spanmetrics
+        // `outcome` dimension has nothing to group these under.
+        //
+        // peer_count is deliberately NOT recorded here, unlike done(): reading
+        // it goes through getPeerCount() -> app_.getOverlay(), and a destructor
+        // must not depend on Overlay still existing. InboundLedgers::stop()
+        // normally clears the map while Overlay is alive, but a shared_ptr held
+        // past that point would destroy this object after Overlay is gone.
+        if (acquireSpan_ && *acquireSpan_)
+        {
+            using namespace telemetry;
+            acquireSpan_->setAttribute(
+                ledger_span::attr::outcome, std::string_view(ledger_span::val::aborted));
+            acquireSpan_->setAttribute(
+                ledger_span::attr::timeouts, static_cast<int64_t>(timeouts_));
+        }
+
         JLOG(journal_.debug()) << "Acquire " << hash_ << " abort "
                                << ((timeouts_ == 0) ? std::string()
                                                     : (std::string("timeouts:") +
@@ -513,6 +534,16 @@ InboundLedger::done()
                 ledger_span::attr::timeouts, static_cast<int64_t>(timeouts_));
             acquireSpan_->setAttribute(
                 ledger_span::attr::peerCount, static_cast<int64_t>(getPeerCount()));
+
+            // Only the failure path gets an Error status. Success is left Unset
+            // rather than Ok: per the OpenTelemetry spec, Ok is reserved for an
+            // application deliberately asserting verified success, and
+            // instrumentation should not set it. The abort path in the
+            // destructor is also left Unset, because a clean shutdown clears
+            // every in-flight acquisition and would otherwise report an error
+            // on every stop.
+            if (failed_)
+                acquireSpan_->setError("ledger acquisition gave up");
         }
 
         JLOG(journal_.debug()) << "Acquire " << hash_ << (failed_ ? " fail " : " ")
diff --git a/src/xrpld/app/ledger/detail/LedgerSpanNames.h b/src/xrpld/app/ledger/detail/LedgerSpanNames.h
index 4de4d24824..361313c4bf 100644
--- a/src/xrpld/app/ledger/detail/LedgerSpanNames.h
+++ b/src/xrpld/app/ledger/detail/LedgerSpanNames.h
@@ -65,6 +65,12 @@ namespace val {
  */
 inline constexpr auto complete = makeStr("complete");
 inline constexpr auto failed = makeStr("failed");
+/**
+ * Set when the acquisition is abandoned before it finishes, i.e. the
+ * InboundLedger is destroyed while !isDone(). Distinct from `failed`, which
+ * means the fetch ran to its retry limit and gave up.
+ */
+inline constexpr auto aborted = makeStr("aborted");
 /**
  * ledger.acquire reason values (mirror InboundLedger::Reason).
  */