diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index 78031187e0..f011f90f76 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -167,6 +167,7 @@ #include #include #include +#include #include #include @@ -541,6 +542,43 @@ public: operator bool() const; }; +// --- Detach-in-place helpers ----------------------------------------------- + +/** + * Detach an active `std::optional` member in place. + * + * Equivalent to `guard.emplace(std::move(*guard).detached())`, which is the + * required idiom for detaching a live guard stored in an `optional` (move + * assignment is deleted because the underlying Scope cannot be re-scoped in + * place). No-op if `guard` is empty or already null. + * + * @param guard The optional guard to detach. Must be called on the thread + * that constructed the active guard inside it (same rule as + * `SpanGuard::detached()`). + * @note Must run BEFORE any `captureContext()` snapshot is taken if the + * caller also needs the pre-detach context — capture first, then call + * this helper (same ordering rule `detached()` itself has). + */ +void +detachInPlace(std::optional& guard); + +/** + * Detach an active `std::shared_ptr`, returning the detached + * guard as a new `shared_ptr`. + * + * Equivalent to + * `guard = std::make_shared(std::move(*guard).detached())`. + * No-op (returns the input unchanged) if `guard` is null or points at a + * null guard. + * + * @param guard The guard to detach, taken by value (the caller's pointer + * is consumed; assign the return value back). + * @return A `shared_ptr` to the detached guard (new allocation), or the + * input pointer unchanged if it was null/inactive. + */ +[[nodiscard]] std::shared_ptr +detachInPlace(std::shared_ptr guard); + // --------------------------------------------------------------------------- // No-op stub (all inline, zero overhead, no OTel dependency) // --------------------------------------------------------------------------- @@ -691,6 +729,19 @@ public: } }; +// --- Detach-in-place helpers (no-op stubs) --------------------------------- + +inline void +detachInPlace(std::optional&) +{ +} + +[[nodiscard]] inline std::shared_ptr +detachInPlace(std::shared_ptr guard) +{ + return guard; +} + #endif // XRPL_ENABLE_TELEMETRY } // namespace xrpl::telemetry diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index c16e48b4d9..f149a5b1ba 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -336,6 +336,24 @@ SpanGuard::detached() && return SpanGuard(std::make_unique(std::move(s), Impl::Detached{})); } +// ===== Detach-in-place helpers ============================================= + +void +detachInPlace(std::optional& guard) +{ + if (!guard || !*guard) + return; + guard.emplace(std::move(*guard).detached()); +} + +std::shared_ptr +detachInPlace(std::shared_ptr guard) +{ + if (!guard || !*guard) + return guard; + return std::make_shared(std::move(*guard).detached()); +} + // ===== Hash-derived span (category-gated) ================================== SpanGuard diff --git a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp index 7c3cdd88ba..afaa5f58a7 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp @@ -38,6 +38,7 @@ #include #include +#include #include #include #include @@ -441,6 +442,108 @@ TEST_F(SpanGuardScopeTest, non_detached_cross_thread_corrupts_origin_stack) EXPECT_EQ(after->GetTraceId(), build->GetTraceId()); } +// detachInPlace(optional&) must detach the live guard the same way the +// hand-rolled emplace(std::move(*opt).detached()) idiom does: after the call +// the guard can be moved to and ended on a worker thread without leaving the +// origin thread's context stack holding it. +TEST_F(SpanGuardScopeTest, detach_in_place_optional_detaches_active_guard) +{ + // Scoped optional guard is active on this (origin) thread. + std::optional opt = SpanGuard::span(TraceCategory::Ledger, "ledger", "build"); + ASSERT_TRUE(opt.has_value()); + ASSERT_TRUE(static_cast(*opt)); + + // Strip the thread-local Scope here on the origin thread via the helper. + detachInPlace(opt); + ASSERT_TRUE(opt.has_value()); + ASSERT_TRUE(static_cast(*opt)); + + // End the detached span on a worker thread. + std::thread worker([d = std::move(*opt)]() mutable {}); + worker.join(); + + // Back on the origin thread: a new span must be a fresh root, proving the + // origin stack top was restored by detachInPlace() (not left holding + // ledger.build). + { + auto after = SpanGuard::span(TraceCategory::Rpc, "rpc", "command"); + ASSERT_TRUE(static_cast(after)); + } + + auto spans = spanData()->GetSpans(); + auto* build = findSpan(spans, "ledger.build"); + auto* after = findSpan(spans, "rpc.command"); + ASSERT_NE(build, nullptr); + ASSERT_NE(after, nullptr); + + // 'after' is a new root: zero parent and a different trace than + // ledger.build. + EXPECT_FALSE(after->GetParentSpanId().IsValid()); + EXPECT_NE(after->GetTraceId(), build->GetTraceId()); +} + +// detachInPlace(optional&) on an empty optional is a no-op: it must not crash, +// must leave the optional empty, and must export nothing. +TEST_F(SpanGuardScopeTest, detach_in_place_optional_noop_on_empty) +{ + std::optional opt; + ASSERT_FALSE(opt.has_value()); + + detachInPlace(opt); + + // Still empty, no span was created or exported. + EXPECT_FALSE(opt.has_value()); + EXPECT_TRUE(spanData()->GetSpans().empty()); +} + +// detachInPlace(shared_ptr) must detach the live guard and return it as a new +// shared_ptr, so the returned guard can be moved to and ended on a worker +// thread without corrupting the origin thread's context stack. +TEST_F(SpanGuardScopeTest, detach_in_place_shared_detaches_active_guard) +{ + // Scoped shared guard is active on this (origin) thread. + auto span = + std::make_shared(SpanGuard::span(TraceCategory::Ledger, "ledger", "build")); + ASSERT_NE(span, nullptr); + ASSERT_TRUE(static_cast(*span)); + + // Strip the thread-local Scope here on the origin thread via the helper. + span = detachInPlace(std::move(span)); + ASSERT_NE(span, nullptr); + ASSERT_TRUE(static_cast(*span)); + + // End the detached span on a worker thread. + std::thread worker([d = std::move(span)]() mutable {}); + worker.join(); + + // Back on the origin thread: a new span must be a fresh root, proving the + // origin stack top was restored by detachInPlace() (not left holding + // ledger.build). + { + auto after = SpanGuard::span(TraceCategory::Rpc, "rpc", "command"); + ASSERT_TRUE(static_cast(after)); + } + + auto spans = spanData()->GetSpans(); + auto* build = findSpan(spans, "ledger.build"); + auto* after = findSpan(spans, "rpc.command"); + ASSERT_NE(build, nullptr); + ASSERT_NE(after, nullptr); + + // 'after' is a new root: zero parent and a different trace than + // ledger.build. + EXPECT_FALSE(after->GetParentSpanId().IsValid()); + EXPECT_NE(after->GetTraceId(), build->GetTraceId()); +} + +// detachInPlace(shared_ptr) on a null pointer is a no-op: it must not crash and +// must return null unchanged. +TEST_F(SpanGuardScopeTest, detach_in_place_shared_noop_on_null) +{ + auto result = detachInPlace(std::shared_ptr{}); + EXPECT_EQ(result, nullptr); +} + } // namespace } // namespace xrpl::telemetry diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index 1eb2c9fd71..f301c4608f 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -540,8 +540,8 @@ RCLConsensus::Adaptor::makeAcceptSpan(Result const& result) // above, so it is not popped on the wrong thread. accept.apply parents // via acceptSpanContext_ (captured above), so no ambient child is // orphaned on either the sync (onForceAccept) or async (onAccept) path. - // Rebuild the shared_ptr from the detached rvalue (move-assign deleted). - span = std::make_shared(std::move(*span).detached()); + // detachInPlace rebuilds the shared_ptr (move-assign is deleted). + span = telemetry::detachInPlace(std::move(span)); } return span; } @@ -1328,9 +1328,9 @@ RCLConsensus::Adaptor::startRoundTracing(RCLCxLedger const& prevLgr) // roundSpanContext_ (captured above) is the durable handle that child // spans on other threads link to. The guard itself is reset() on a // different worker than it was emplaced on, so detach its Scope now -- - // AFTER the context capture -- to avoid a wrong-thread Scope pop. Re-emplace - // the detached guard in place (SpanGuard move-assignment is deleted). - roundSpan_.emplace(std::move(*roundSpan_).detached()); + // AFTER the context capture -- to avoid a wrong-thread Scope pop. + // detachInPlace re-emplaces it (move-assignment is deleted). + telemetry::detachInPlace(roundSpan_); } std::optional diff --git a/src/xrpld/consensus/Consensus.h b/src/xrpld/consensus/Consensus.h index 917bf84463..df81c7702a 100644 --- a/src/xrpld/consensus/Consensus.h +++ b/src/xrpld/consensus/Consensus.h @@ -2116,7 +2116,7 @@ Consensus::startEstablishTracing() if (*establishSpan_) { establishSpanContext_ = establishSpan_->captureContext(); - establishSpan_.emplace(std::move(*establishSpan_).detached()); + telemetry::detachInPlace(establishSpan_); } }