From deecae0f0199a131d2245f34366660a055add609 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 20:15:31 +0100 Subject: [PATCH] refactor(telemetry): tx spans thread-free (no detach); txq spans scoped for nesting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SpanGuard is now thread-free (holds no Scope), so the tx.receive (PeerImp) and tx.process (NetworkOPs) handoff sites no longer need .detached() before being stored and ended on a worker thread — just construct the guard. The stale "Scope leak" comments are replaced accordingly. Make the six txq.* spans ScopedSpanGuard so their sub-spans nest via the ambient context: txq.apply_direct/batch_clear under txq.enqueue, and txq.accept_tx under txq.accept. All six are verified synchronous, ended at scope, and never moved/handed off, so scoping is safe. applyDirect's span is pure RAII (no method calls), so it is declared const. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/app/misc/NetworkOPs.cpp | 9 ++++----- src/xrpld/app/misc/detail/TxQ.cpp | 15 ++++++--------- src/xrpld/overlay/detail/PeerImp.cpp | 7 +++---- 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index bffe8c5c9f..aa5a483119 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1384,11 +1384,10 @@ NetworkOPsImp::processTransaction( FailHard failType) { using namespace telemetry; - // Detached: this span is stored in TransactionStatus and applied on a - // batch worker thread, so it must not leave its Scope bound to this - // thread's context stack (that leak would adopt later work into this - // transaction's trace). - auto span = std::make_shared(txProcessSpan(transaction->getID()).detached()); + // SpanGuard is thread-free (holds no Scope), so it is safe to store here + // and end on the batch worker thread that later applies this transaction — + // no detach step is needed. + auto span = std::make_shared(txProcessSpan(transaction->getID())); span->setAttribute(tx_span::attr::txHash, to_string(transaction->getID()).c_str()); span->setAttribute(tx_span::attr::local, bLocal); if (auto const& stx = transaction->getSTransaction()) diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index d3a6b3290d..e0c988a418 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -545,7 +545,7 @@ TxQ::tryClearAccountQueueUpThruTx( beast::Journal j) { using namespace telemetry; - [[maybe_unused]] auto span = SpanGuard::span( + [[maybe_unused]] ScopedSpanGuard span( TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::batchClear); SeqProxy const tSeqProx{tx.getSeqProxy()}; @@ -752,8 +752,7 @@ TxQ::apply( beast::Journal j) { using namespace telemetry; - auto span = - SpanGuard::span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::enqueue); + ScopedSpanGuard span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::enqueue); span.setAttribute(txq_span::attr::txHash, to_string(tx->getTransactionID()).c_str()); if (auto const* fmt = TxFormats::getInstance().findByType(tx->getTxnType())) span.setAttribute(txq_span::attr::txType, fmt->getName().c_str()); @@ -1374,8 +1373,7 @@ void TxQ::processClosedLedger(Application& app, ReadView const& view, bool timeLeap) { using namespace telemetry; - auto span = - SpanGuard::span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::cleanup); + ScopedSpanGuard span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::cleanup); span.setAttribute(txq_span::attr::ledgerSeq, static_cast(view.header().seq)); std::scoped_lock const lock(mutex_); @@ -1466,8 +1464,7 @@ TxQ::accept(Application& app, OpenView& view) // Create the span and read byFee_.size() only after taking the lock, since // byFee_ is guarded by mutex_. - auto span = - SpanGuard::span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::accept); + ScopedSpanGuard span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::accept); span.setAttribute(txq_span::attr::queueSize, static_cast(byFee_.size())); auto const metricsSnapshot = feeMetrics_.getSnapshot(); @@ -1497,7 +1494,7 @@ TxQ::accept(Application& app, OpenView& view) JLOG(j_.trace()) << "Applying queued transaction " << candidateIter->txID << " to open ledger."; - auto txSpan = SpanGuard::span( + ScopedSpanGuard txSpan( TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::acceptTx); txSpan.setAttribute(txq_span::attr::txHash, to_string(candidateIter->txID).c_str()); txSpan.setAttribute( @@ -1718,7 +1715,7 @@ TxQ::tryDirectApply( beast::Journal j) { using namespace telemetry; - [[maybe_unused]] auto span = SpanGuard::span( + [[maybe_unused]] ScopedSpanGuard const span( TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::applyDirect); auto const account = (*tx)[sfAccount]; diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index c0fb9c6c56..fcf07046e4 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1311,10 +1311,9 @@ PeerImp::handleTransaction( uint256 const txID = stx->getTransactionID(); using namespace telemetry; - // Detached: this span is handed to a job-queue worker and must not - // leave its Scope bound to this peer thread's context stack (that - // leak would adopt later peer messages into this transaction's trace). - auto span = std::make_shared(txReceiveSpan(txID, *m).detached()); + // SpanGuard is thread-free (holds no Scope), so it is safe to hand to + // a job-queue worker and end on that thread — no detach step is needed. + auto span = std::make_shared(txReceiveSpan(txID, *m)); span->setAttribute(tx_span::attr::txHash, to_string(txID).c_str()); span->setAttribute(tx_span::attr::peerId, static_cast(id_)); if (auto const* fmt = TxFormats::getInstance().findByType(stx->getTxnType()))