Merge branch 'pratik/otel-phase3-tx-tracing' into pratik/otel-phase4-consensus-tracing

include/xrpl/telemetry/SpanGuard.h

@@ -149,6 +149,10 @@
  * it may be moved to and destroyed on another thread; detached()
  * itself must be called on the origin (constructing) thread. Use
  * captureContext() to propagate the trace context to other threads.
+ * Violating this rule is enforced (not just documented): a scoped
+ * guard destroyed on a foreign thread, or detached() called from one,
+ * trips an XRPL_ASSERT in debug/test/fuzzing builds instead of
+ * silently corrupting the other thread's context stack.
  *
  * @note Move semantics: Move construction transfers ownership of
  * the pimpl pointer — no double-Scope issues. Move assignment is
@@ -364,7 +368,8 @@ public:
      *
      * @return A scope-less guard safe to move to and destroy on
      * another thread, or a null guard if this guard was null.
-     * @note Must be called on the origin (constructing) thread. The
+     * @note Must be called on the origin (constructing) thread; this is
+     * checked by an XRPL_ASSERT in debug/test/fuzzing builds. The
      * returned guard may be freely moved across threads; only
      * its final destruction ends the span.
      */

src/libxrpl/telemetry/SpanGuard.cpp

@@ -26,6 +26,7 @@
 #include <xrpl/telemetry/SpanGuard.h>
 
 #include <xrpl/basics/random.h>
+#include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/telemetry/DiscardFlag.h>
 #include <xrpl/telemetry/Telemetry.h>
 #include <xrpl/telemetry/TraceContextPropagator.h>
@@ -54,6 +55,7 @@
 #include <optional>
 #include <string>
 #include <string_view>
+#include <thread>
 #include <typeinfo>
 #include <utility>
 #include <vector>
@@ -100,6 +102,16 @@ struct SpanGuard::Impl
      */
     std::optional<otel_trace::Scope> scope;
 
+    /**
+     * Thread that constructed this Impl. Meaningful only when `scope`
+     * holds a value: a Scope is bound to the thread-local context stack
+     * of the thread that pushed it, so popping it (via ~Scope, when
+     * ~Impl runs) on a different thread corrupts that thread's stack.
+     * Checked in ~Impl() and detached() to turn a silent cross-thread
+     * corruption into an assertion failure in debug/test/fuzzing builds.
+     */
+    std::thread::id owner{std::this_thread::get_id()};
+
     /**
      * Construct a scoped guard: the span is pushed onto this thread's
      * active-context stack for the lifetime of the guard.
@@ -129,6 +141,14 @@ struct SpanGuard::Impl
 
     ~Impl()
     {
+        // A live Scope (scope engaged) must be popped on the thread that
+        // pushed it; ending on any other thread corrupts that thread's
+        // context stack. A detached guard (scope == nullopt) carries no
+        // binding and may be destroyed anywhere, so the check is skipped.
+        XRPL_ASSERT(
+            !scope.has_value() || owner == std::this_thread::get_id(),
+            "xrpl::telemetry::SpanGuard::Impl::~Impl : scoped guard destroyed on "
+            "constructing thread");
         if (span)
             span->End();
     }
@@ -327,6 +347,11 @@ SpanGuard::detached() &&
 {
     if (!impl_)
         return {};
+    // Popping the Scope (below, via impl_.reset()) must happen on the thread
+    // that pushed it; calling detached() elsewhere would pop the wrong stack.
+    XRPL_ASSERT(
+        !impl_->scope.has_value() || impl_->owner == std::this_thread::get_id(),
+        "xrpl::telemetry::SpanGuard::detached : called on constructing thread");
     // Take the span out; the old Impl.span is now null so ~Impl won't End().
     auto s = std::move(impl_->span);
     // Resetting the old Impl destroys its Scope HERE, on the origin thread,

src/tests/libxrpl/telemetry/SpanGuardScope.cpp

@@ -392,38 +392,68 @@ TEST_F(SpanGuardScopeTest, root_then_detached_is_root_and_leaves_stack_clean)
     EXPECT_EQ(child->GetTraceId(), ambient->GetTraceId());
 }
 
-// Negative control (documents the bug detached() fixes): a scoped guard NOT
-// detached, moved to and destroyed on a worker thread, pops the WRONG thread's
-// stack, so a later span on the origin thread inherits the stale span. The
-// whole scenario runs on a dedicated origin thread so the corrupted
-// thread-local stack cannot leak into other tests.
-TEST_F(SpanGuardScopeTest, non_detached_cross_thread_corrupts_origin_stack)
+// Death test guarding the cross-thread scope-leak bug.
+//
+// This used to be a "negative control": a scoped guard was NOT detached, moved
+// to and destroyed on a worker thread (popping the WRONG thread's stack), and
+// the test then asserted that a later span on the origin thread had SILENTLY
+// inherited the stale span -- proving the corruption happened undetected.
+//
+// SpanGuard::Impl::~Impl now enforces thread affinity with an XRPL_ASSERT: a
+// live scope must be destroyed on the thread that created it. In debug/test
+// builds (NDEBUG unset) XRPL_ASSERT expands to assert(), so the exact sequence
+// above now aborts the process inside the worker thread's destructor instead of
+// corrupting the origin stack. The bug therefore moved from "silently wrong" to
+// "loudly crashes early" -- the intended tradeoff, since a crash is far easier
+// to catch than a corrupted trace hierarchy.
+//
+// The death happens on the WORKER thread (the moved-in guard is destroyed when
+// the worker lambda returns, before worker.join() completes). A failed assert()
+// calls abort(), which raises SIGABRT process-wide regardless of which thread
+// hit it, so EXPECT_DEATH -- which runs the statement in a forked child and
+// checks it dies -- observes the crash. The regex matches the assert message
+// substring rather than the whole line, because the file:line/function prefix
+// glibc prints around it is platform and compiler dependent.
+//
+// The test is skipped where the assertion cannot fire: under NDEBUG (Release
+// builds) XRPL_ASSERT is a no-op, and under ENABLE_VOIDSTAR a failed assert
+// continues instead of aborting -- in both cases the worker would not crash and
+// EXPECT_DEATH would report a spurious failure.
+TEST_F(SpanGuardScopeTest, non_detached_cross_thread_death_asserts_at_wrong_thread_destroy)
 {
-    std::thread origin([&] {
-        auto guard = SpanGuard::span(TraceCategory::Ledger, "ledger", "build");
+    // XRPL_ASSERT only aborts when assertions are live. Under NDEBUG (Release
+    // builds) it expands to assert(), which the C standard strips to a no-op,
+    // so the worker thread destroys the guard without crashing. Under
+    // ENABLE_VOIDSTAR (Antithesis fuzzing) a failed XRPL_ASSERT continues
+    // execution instead of aborting. In either case the process does not die
+    // and EXPECT_DEATH would fail, so skip this test there. The two macros are
+    // mutually exclusive (instrumentation.h #errors on ENABLE_VOIDSTAR+NDEBUG),
+    // but both break the death check, so guard against each.
+#ifdef NDEBUG
+    GTEST_SKIP() << "XRPL_ASSERT compiles to a no-op under NDEBUG (Release builds), so the "
+                    "cross-thread scope-leak assertion this test exercises does not fire.";
+#elif defined(ENABLE_VOIDSTAR)
+    GTEST_SKIP() << "ENABLE_VOIDSTAR continues past a failed XRPL_ASSERT instead of aborting, so "
+                    "the cross-thread scope-leak assertion this test exercises does not crash.";
+#else
+    EXPECT_DEATH(
         {
-            // ~SpanGuard on the worker ends the span and Detaches on the
-            // WORKER stack -- the origin stack is never popped.
+            // Scoped guard is active on this (origin) thread; its Scope is
+            // bound to this thread.
+            auto guard = SpanGuard::span(TraceCategory::Ledger, "ledger", "build");
+
+            // Move the still-scoped guard to a worker thread WITHOUT detaching.
+            // ~SpanGuard::Impl runs on the worker when the lambda returns and
+            // trips the thread-affinity assertion -> abort().
             std::thread worker([d = std::move(guard)]() mutable {});
             worker.join();
-        }
-        // Origin stack still holds the stale ledger.build as active, so this
-        // span inherits it as parent instead of being a root.
-        auto after = SpanGuard::span(TraceCategory::Rpc, "rpc", "command");
-        ASSERT_TRUE(static_cast<bool>(after));
-    });
-    origin.join();
-
-    auto spans = spanData()->GetSpans();
-    auto* build = findSpan(spans, "ledger.build");
-    auto* after = findSpan(spans, "rpc.command");
-    ASSERT_NE(build, nullptr);
-    ASSERT_NE(after, nullptr);
-
-    // BUG: 'after' inherited the stale span -- non-zero parent, same trace.
-    EXPECT_TRUE(after->GetParentSpanId().IsValid());
-    EXPECT_EQ(after->GetParentSpanId(), build->GetSpanId());
-    EXPECT_EQ(after->GetTraceId(), build->GetTraceId());
+        },
+        // The assert message is written as two adjacent string literals in
+        // SpanGuard.cpp; glibc's assert() stringifies the expression source via
+        // the preprocessor '#' operator, keeping both quoted literals with the
+        // "\" \"" gap between "on" and "constructing". Match across that gap.
+        ".*scoped guard destroyed on.*constructing thread.*");
+#endif
 }
 
 // detachInPlace(optional&) must detach the live guard the same way the