Fix race conditions on hub and span

[giortzisg]

Closes both #1048 and #1049.

Description

Adds tests for Hubs, Scopes and Spans to identify race conditions and fixes everything uncovered.

Changes include:

• Add getAtomicClientAndScope. Previously when accessing the client and scope it was possible for one of them to change between the two separate calls. Should be accessed under a single lock.
• Add scope locks when parsing trace and baggage

[codecov]

Codecov Report

Attention: Patch coverage is 92.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.09%. Comparing base (221b2ac) to head (dc65a13).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
hub.go	90.32%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1050      +/-   ##
==========================================
+ Coverage   86.71%   87.09%   +0.38%     
==========================================
  Files          55       55              
  Lines        5804     5859      +55     
==========================================
+ Hits         5033     5103      +70     
+ Misses        629      613      -16     
- Partials      142      143       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lcian]

Suggested change

	scope.mu.RLock()
	defer scope.mu.RUnlock()
	if scope.span != nil {
	return scope.span.ToBaggage()
	span := scope.span
	if span != nil {
	return span.ToBaggage()
	}
	if span := scope.GetSpan(); span != nil {
	return span.ToBaggage()
	}

[lcian]

scope.GetSpan() already does the locking. Additionally we can do it like this which afaik is more idiomatic :)

[giortzisg]

Actually just remembered why I did the change like this. After the if block we are still accessing scope.propagationContext so we need to lock the scope again.

[lcian]

Suggested change

	scope.mu.RLock()
	defer scope.mu.RUnlock()
	if scope.span != nil {
	return scope.span.ToSentryTrace()
	span := scope.span
	if span != nil {
	return span.ToSentryTrace()
	}
	if span := scope.GetSpan(); span != nil {
	return span.ToSentryTrace()
	}

[giortzisg]

Same as above comment.

[lcian]

There seem to be some more places where both the hub and scope are used without using the new function which could possibly still lead to race conditions, as an example:

• gin/sentrygin.go in the handle function
• fiber/sentryfiber.go in the handle function
• dynamic_sampling_context.go
  etc.

I would suggest reviewing all usages of hub.Client() and hub.Scope() and checking whether they need to be corrected.

[cursor]

Bug: Race Condition in `ToBaggage()` Method

The ToBaggage() method introduces a race condition. It acquires a read lock on the current span (s) but then attempts to modify the dynamicSamplingContext field of the transaction span (t = s.GetTransaction()). This write operation is performed without holding a write lock on t. If t is s, the write occurs under s's read lock, violating the read-write mutex contract. If t is a different span, t's lock is not held at all, leading to concurrent writes without proper synchronization.

tracing.go#L326-L342

sentry-go/tracing.go

Lines 326 to 342 in 4ac25ad

	func (s *Span) ToBaggage() string {
	s.mu.RLock()
	defer s.mu.RUnlock()
	t := s.GetTransaction()
	if t == nil {
	return ""
	}
	// In case there is currently no frozen DynamicSamplingContext attached to the transaction,
	// create one from the properties of the transaction.
	if !s.dynamicSamplingContext.IsFrozen() {
	// This will return a frozen DynamicSamplingContext.
	if dsc := DynamicSamplingContextFromTransaction(t); dsc.HasEntries() {
	t.dynamicSamplingContext = dsc
	}
	}

Fix in Cursor • Fix in Web

---

Was this report helpful? Give feedback by reacting with 👍 or 👎