fix(telemetry): assert SpanGuard Scope is destroyed on its constructing thread

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Pratik Mankawde
2026-07-21 11:26:29 +01:00
parent b9c049d7c6
commit f9b2d725a0
2 changed files with 31 additions and 1 deletions

View File

@@ -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
@@ -333,7 +337,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.
*/

View File

@@ -25,6 +25,7 @@
#include <xrpl/telemetry/SpanGuard.h>
#include <xrpl/beast/utility/instrumentation.h>
#include <xrpl/telemetry/DiscardFlag.h>
#include <xrpl/telemetry/Telemetry.h>
@@ -44,6 +45,7 @@
#include <optional>
#include <string>
#include <string_view>
#include <thread>
#include <typeinfo>
#include <utility>
@@ -89,6 +91,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.
@@ -118,6 +130,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();
}
@@ -312,6 +332,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,