Fix race conditions on log when accessing scope #1050

giortzisg · 2025-07-02T10:50:29Z

Closes both #1048 and #1049.

Description

Adds tests for the logger to identify race conditions and fix everything uncovered.

Changes include:

Add locks when using SetAttributes to guard for panics.
Lock scope when trying to access propagation context on log.

codecov · 2025-07-02T10:52:02Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.08%. Comparing base (17421c4) to head (0a2c20d).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1050      +/-   ##
==========================================
+ Coverage   86.88%   87.08%   +0.20%     
==========================================
  Files          55       55              
  Lines        5956     5971      +15     
==========================================
+ Hits         5175     5200      +25     
+ Misses        638      630       -8     
+ Partials      143      141       -2

☔ 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.

hub.go

lcian · 2025-07-07T13:58:21Z

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.

lcian

Mostly looks good, please see the comments

transport_test.go

log_test.go

log.go

giortzisg · 2025-08-07T09:31:15Z

@lcian For the cursor comment about nil scope. If you clone the Hub and scope should never be empty. We can still guard with nil checks on the scope.Set methods, but it should be ok as is right now also.

lcian · 2025-08-07T09:39:06Z

@lcian For the cursor comment about nil scope. If you clone the Hub and scope should never be empty. We can still guard with nil checks on the scope.Set methods, but it should be ok as is right now also.

Yep I see. I was talking about this one: #1050 (comment)

cleptric · 2025-08-11T07:43:20Z

hub_test.go

@@ -553,3 +555,19 @@ func TestHub_FlushWithContext(t *testing.T) {
 		t.Fatalf("expected message to be %v, got %v", wantEvent.Message, gotEvents[0].Message)
 	}
 }
+
+func TestHubEmptyClientShouldBeSafe(t *testing.T) {


What does this mean? 😬

That the hub shouldn't panic with an empty client 😅

Maybe we use a a test name with proper grammar then?

giortzisg requested a review from cleptric July 2, 2025 10:50

giortzisg self-assigned this Jul 2, 2025

giortzisg added the Bug Issue type label Jul 2, 2025