From 7dbbf95c72cb56eebd5f00f69b0246363d3a0bbd Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 18:52:48 +0100 Subject: [PATCH] refactor(telemetry): ScopedSpanGuard asserts same context store, not thread Coroutine-aware storage lets a scope resume on another worker within the same coroutine store; store-identity is the correct pop-safety invariant. Same-store equals same-thread for synchronous code, so no safety is lost. Co-Authored-By: Claude Opus 4.8 (1M context) --- include/xrpl/telemetry/SpanGuard.h | 47 ++++++++------- src/libxrpl/telemetry/SpanGuard.cpp | 88 ++++++++++++++++++----------- 2 files changed, 82 insertions(+), 53 deletions(-) diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index 341dc9fe71..d0abaa3769 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -11,9 +11,11 @@ * to and destroyed on any thread. Movable AND * move-assignable. * - ScopedSpanGuard — owns a SpanGuard plus an active OTel Scope that - * pushes the span onto the constructing thread's - * context stack. Thread-bound: it must be created and - * destroyed on the same thread. Non-copyable and + * pushes the span onto the constructing context store's + * stack. Store-bound: it must be created and destroyed + * while the same LocalValue context store is active (the + * same thread, or the same JobQueue coroutine even if it + * resumes on another worker). Non-copyable and * non-movable. * * Both wrap all OpenTelemetry types behind the pimpl idiom so no @@ -446,26 +448,26 @@ public: }; /** - * RAII guard that activates a span on the current thread (scoped). + * RAII guard that activates a span on the current context store (scoped). * * Wraps a SpanGuard (which owns the span) plus an OTel Scope that - * pushes the span onto this thread's thread-local context stack for - * the guard's lifetime, so child spans created on the thread inherit + * pushes the span onto the active context store's stack for the + * guard's lifetime, so child spans created under that store inherit * it as parent. On destruction the Scope pops BEFORE the span ends * (member order: guard first, scope second). Non-copyable and * non-movable — factories return unnamed temporaries, so guaranteed * copy elision (C++17) covers `auto s = ScopedSpanGuard::freshRoot(...)`. * - * When the span must outlive the current thread (e.g. handed to a job + * When the span must outlive the current store (e.g. handed to a job * queue), convert to a plain SpanGuard with `operator SpanGuard() &&`: - * that pops the Scope eagerly on the origin thread and yields a + * that pops the Scope eagerly under the constructing store and yields a * thread-free guard. * * ScopedSpanGuard dependency diagram: * * +--------------------------------------------+ * | ScopedSpanGuard | - * | (scoped, thread-bound) | + * | (scoped, store-bound) | * +--------------------------------------------+ * | - impl_ : unique_ptr (pimpl) | * +--------------------------------------------+ @@ -483,7 +485,7 @@ public: * | | * +----------+ +-----------------------------+ * | SpanGuard| | optional | - * | (span) | | (OTel, thread-bound; | + * | (span) | | (OTel, store-bound; | * | | | present : span active) | * +----------+ +-----------------------------+ * @@ -513,11 +515,14 @@ public: * @endcode * * @note Thread safety: A ScopedSpanGuard must be constructed AND - * destroyed on the same thread — its Scope binds to that thread's - * context stack. `operator SpanGuard() &&` and the destructor must - * both run on the owning thread; both check this with an XRPL_ASSERT - * in debug/test/fuzzing builds. `operator SpanGuard() &&` pops the - * scope eagerly, so the resulting SpanGuard is thread-free. + * destroyed while the same LocalValue context store is active — its + * Scope binds to that store's context stack. This means the same + * thread, or the same JobQueue coroutine even if it resumes on another + * worker (coroutine-aware storage keeps the same store across the + * resume). `operator SpanGuard() &&` and the destructor must both run + * under the constructing store; both check this with an XRPL_ASSERT in + * debug/test/fuzzing builds. `operator SpanGuard() &&` pops the scope + * eagerly, so the resulting SpanGuard is thread-free. * * @note Known limitations: * - Non-movable: cannot be stored in a container that relocates, and @@ -609,9 +614,10 @@ public: // --- Handoff bridge ------------------------------------------------- /** - * Pop the active Scope on the origin thread and yield the span as a - * thread-free SpanGuard. Use to move a span into a job or onto - * another thread. Must be called on the constructing thread. + * Pop the active Scope under the constructing context store and yield + * the span as a thread-free SpanGuard. Use to move a span into a job or + * onto another thread. Must be called while the constructing context + * store is active. * @return The unscoped SpanGuard that now owns the span. */ operator SpanGuard() &&; @@ -692,8 +698,9 @@ public: /** * Mark this span for discard and end it immediately. discard() pops the - * thread-local scope right away (on the owning thread) and discards the - * span. After discard() the guard is inert and its destructor is a no-op. + * scope right away (under the constructing context store) and discards + * the span. After discard() the guard is inert and its destructor is a + * no-op. */ void discard(); diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index 8c71e55046..744f93a35c 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -10,10 +10,12 @@ * touches the thread-local context stack, so a SpanGuard is * thread-free and may be moved to and destroyed on any thread. * - ScopedSpanGuard::ScopedImpl holds a SpanGuard plus an optional - * Scope. The Scope pushes the span onto the constructing thread's - * context stack; member order (guard first, scope second) ensures + * Scope. The Scope pushes the span onto the constructing context + * store's stack; member order (guard first, scope second) ensures * the Scope pops BEFORE the span ends on destruction. It is - * thread-bound: construct and destroy on the same thread. + * store-bound: construct and destroy while the same LocalValue + * context store is active (the same thread, or the same JobQueue + * coroutine even if it resumes on another worker). * * Static factory methods access the global Telemetry instance via * Telemetry::getInstance(), check whether the requested TraceCategory @@ -28,6 +30,7 @@ #include +#include #include #include #include @@ -48,7 +51,6 @@ #include #include #include -#include #include #include @@ -428,21 +430,35 @@ struct ScopedSpanGuard::ScopedImpl SpanGuard guard; /** - * Active OTel Scope binding the span to this thread's context stack. - * Present while the guard is active; reset() (via operator SpanGuard) - * pops it eagerly on the origin thread. Declared AFTER `guard` so - * destruction order pops the scope before the span ends. + * Active OTel Scope binding the span to the active context store's + * stack. Present while the guard is active; reset() (via operator + * SpanGuard) pops it eagerly under the constructing store. Declared + * AFTER `guard` so destruction order pops the scope before the span + * ends. */ std::optional scope; /** - * Thread that constructed this ScopedImpl. The Scope is bound to - * this thread's context stack, so popping it (via ~Scope or reset()) - * on a different thread corrupts that thread's stack. Checked in the - * destructor and operator SpanGuard() to turn a silent cross-thread - * corruption into an assertion failure in debug/test/fuzzing builds. + * Identity of the LocalValue store the Scope was pushed onto (the + * coroutine's store on a coro, else the thread's own store). Popping + * the Scope (via ~Scope or reset()) must happen while the SAME store + * is active, else a different stack is corrupted. Coroutine-aware + * storage lets a coro legitimately resume on another worker while + * keeping the same store, so store-identity — not thread-id — is the + * correct invariant. Checked in the destructor and operator + * SpanGuard() to turn a silent cross-store pop into an assertion + * failure in debug/test/fuzzing builds. + * + * Assigned in the constructor body AFTER the Scope push, not as a + * member initializer: the push is the first LocalValue touch on a + * fresh thread for a root or explicit-parent span (the OTel SDK skips + * the ambient-context read in those cases), so it materializes the + * store. Capturing before the push would record nullptr while the + * destructor sees the now-materialized store. For a null guard no + * Scope is pushed and the assertions short-circuit, so this holds + * whatever store is active then. */ - std::thread::id owner{std::this_thread::get_id()}; + void const* owner = nullptr; /** * Wrap a SpanGuard and activate its span on this thread. If the @@ -454,6 +470,9 @@ struct ScopedSpanGuard::ScopedImpl { if (guard) scope.emplace(guard.impl_->span); + // Capture after the push so `owner` names the store the Scope + // actually lives on, even when the push materialized it. + owner = detail::getLocalValues().get(); } }; @@ -466,13 +485,14 @@ ScopedSpanGuard::ScopedSpanGuard(SpanGuard&& guard) ScopedSpanGuard::~ScopedSpanGuard() { - // A live Scope must be popped on the thread that pushed it; destroying - // the guard on any other thread corrupts that thread's context stack. - // A null guard holds no Scope, so the check is skipped. + // A live Scope must be popped while its constructing context store is + // active; destroying the guard under a different store corrupts that + // store's context stack. A null guard holds no Scope, so the check is + // skipped. XRPL_ASSERT( - !impl_ || !impl_->scope.has_value() || impl_->owner == std::this_thread::get_id(), - "xrpl::telemetry::ScopedSpanGuard::~ScopedSpanGuard : destroyed on " - "constructing thread"); + !impl_ || !impl_->scope.has_value() || impl_->owner == detail::getLocalValues().get(), + "xrpl::telemetry::ScopedSpanGuard::~ScopedSpanGuard : destroyed on the " + "constructing context store"); } // ===== ScopedSpanGuard factory methods ===================================== @@ -517,13 +537,14 @@ ScopedSpanGuard::linkedSpan(std::string_view name, SpanContext const& linkCtx) ScopedSpanGuard:: operator SpanGuard() && { - // The scope must be popped on the thread that pushed it; handing off - // elsewhere would pop the wrong stack. + // The scope must be popped while its constructing context store is + // active; handing off under a different store would pop the wrong stack. + // A null guard holds no Scope, so the check is skipped. XRPL_ASSERT( - impl_->owner == std::this_thread::get_id(), - "xrpl::telemetry::ScopedSpanGuard::operator SpanGuard : handoff on " - "constructing thread"); - impl_->scope.reset(); // eager pop, on the origin thread + !impl_->scope.has_value() || impl_->owner == detail::getLocalValues().get(), + "xrpl::telemetry::ScopedSpanGuard::operator SpanGuard : handoff on the " + "constructing context store"); + impl_->scope.reset(); // eager pop, on the origin store return std::move(impl_->guard); } @@ -594,14 +615,15 @@ ScopedSpanGuard::recordException(std::exception const& e) void ScopedSpanGuard::discard() { - // A live Scope must be popped on the thread that pushed it; discarding - // on any other thread corrupts that thread's context stack. A null guard - // holds no Scope, so the check is skipped. + // A live Scope must be popped while its constructing context store is + // active; discarding under a different store corrupts that store's + // context stack. A null guard holds no Scope, so the check is skipped. XRPL_ASSERT( - !impl_ || !impl_->scope.has_value() || impl_->owner == std::this_thread::get_id(), - "xrpl::telemetry::ScopedSpanGuard::discard : discarded on constructing thread"); - // Pop the scope first (on this thread) so no span stays active on the - // stack, then discard the span via the owned guard. + !impl_ || !impl_->scope.has_value() || impl_->owner == detail::getLocalValues().get(), + "xrpl::telemetry::ScopedSpanGuard::discard : discarded on the " + "constructing context store"); + // Pop the scope first (under the constructing store) so no span stays + // active on the stack, then discard the span via the owned guard. impl_->scope.reset(); impl_->guard.discard(); }