Skip to content

fix(serialization): pre-serialize mutable event fields to prevent race panics #1214

Merged
giortzisg merged 3 commits intomasterfrom
fix/safe-serialization
Mar 3, 2026
Merged

fix(serialization): pre-serialize mutable event fields to prevent race panics #1214
giortzisg merged 3 commits intomasterfrom
fix/safe-serialization

Conversation

@giortzisg
Copy link
Contributor

Description

This PR Fixes #1146.

json.Marshal on the background scheduler goroutine can panic with "index out of range" when user code concurrently mutates Event fields containing maps or slices (Extra, Contexts, Breadcrumbs, Exception,
User.Data).

Instead of deep-copying these fields, this pre-serializes them to json.RawMessage in MakeSerializationSafe() on the calling goroutine. The background MarshalJSON then emits the frozen bytes via a shadow struct, avoiding any access to the live shared data.

Key decisions:

  • TelemetryItem interface: MakeSerializationSafe() is called in Processor.Add() so all telemetry types are automatically protected
  • No double serialization: mutable fields are marshaled once to []byte; non-mutable fields are still serialized on the background goroutine

Issues

Changelog Entry Instructions

To add a custom changelog entry, uncomment the section above. Supports:

  • Single entry: just write text
  • Multiple entries: use bullet points
  • Nested bullets: indent 4+ spaces

For more details: custom changelog entries

Reminders

@giortzisg giortzisg self-assigned this Mar 2, 2026
@linear
Copy link

linear bot commented Mar 2, 2026

@github-actions
Copy link

github-actions bot commented Mar 2, 2026

Semver Impact of This PR

🟢 Patch (bug fixes)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


New Features ✨

  • Add the option to set attributes on the scope by giortzisg in #1208

Bug Fixes 🐛

  • (serialization) Pre-serialize mutable event fields to prevent race panics by giortzisg in #1214

Internal Changes 🔧

Deps

  • Bump getsentry/craft from 2.20.1 to 2.23.1 by dependabot in #1213
  • Bump github.com/gofiber/fiber/v2 from 2.52.11 to 2.52.12 in /fiber by dependabot in #1209

Other

  • (ai) Add dotagents configuration by giortzisg in #1211

🤖 This preview updates automatically when you update the PR.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Transaction spans silently dropped in pre-serialized path
    • I removed the safeTransaction shadow Spans field so transaction marshaling now keeps Event.Spans in the pre-serialized path.
  • ✅ Fixed: User silently dropped when User.Data is empty
    • I changed MakeSerializationSafe to serialize User whenever it is non-empty (!e.User.IsEmpty()), preserving users without Data.

Create PR

Or push these changes by commenting:

@cursor push 2b9dc34904
Preview (2b9dc34904)
diff --git a/interfaces.go b/interfaces.go
--- a/interfaces.go
+++ b/interfaces.go
@@ -641,7 +641,6 @@
 			Contexts    json.RawMessage `json:"contexts,omitempty"`
 			Breadcrumbs json.RawMessage `json:"breadcrumbs,omitempty"`
 			Exception   json.RawMessage `json:"exception,omitempty"`
-			Spans       json.RawMessage `json:"spans,omitempty"`
 			User        json.RawMessage `json:"user,omitempty"`
 		}
 		return json.Marshal(safeTransaction{
@@ -904,7 +903,7 @@
 		}
 	}
 
-	if len(e.User.Data) > 0 {
+	if !e.User.IsEmpty() {
 		if b, err := json.Marshal(e.User); err == nil {
 			e.serializedUser = b
 		}

diff --git a/interfaces_test.go b/interfaces_test.go
--- a/interfaces_test.go
+++ b/interfaces_test.go
@@ -1028,6 +1028,73 @@
 		}
 	})
 
+	t.Run("pre-serializes User when Data is empty", func(t *testing.T) {
+		event := &Event{
+			Extra: map[string]interface{}{"key": "value"},
+			User:  User{ID: "1", Email: "user@example.com"},
+		}
+		event.MakeSerializationSafe()
+
+		if event.serializedUser == nil {
+			t.Fatal("serializedUser should be set")
+		}
+
+		jsonData, err := json.Marshal(event)
+		if err != nil {
+			t.Fatalf("marshal failed: %v", err)
+		}
+
+		var got map[string]interface{}
+		if err := json.Unmarshal(jsonData, &got); err != nil {
+			t.Fatalf("unmarshal failed: %v", err)
+		}
+
+		user, ok := got["user"].(map[string]interface{})
+		if !ok {
+			t.Fatalf("expected user field in JSON, got %s", jsonData)
+		}
+		if user["id"] != "1" {
+			t.Errorf("expected user.id=1, got %v", user["id"])
+		}
+		if user["email"] != "user@example.com" {
+			t.Errorf("expected user.email=user@example.com, got %v", user["email"])
+		}
+	})
+
+	t.Run("pre-serializes transaction spans", func(t *testing.T) {
+		event := &Event{
+			Type: transactionType,
+			Contexts: map[string]Context{
+				"trace": TraceContext{
+					TraceID: TraceIDFromHex("90d57511038845dcb4164a70fc3a7fdb"),
+					SpanID:  SpanIDFromHex("f7f3fd754a9040eb"),
+				}.Map(),
+			},
+			Spans: []*Span{{
+				TraceID:   TraceIDFromHex("90d57511038845dcb4164a70fc3a7fdb"),
+				SpanID:    SpanIDFromHex("4aaf45ea7db94520"),
+				StartTime: time.Unix(1, 0).UTC(),
+				EndTime:   time.Unix(2, 0).UTC(),
+			}},
+		}
+		event.MakeSerializationSafe()
+
+		jsonData, err := json.Marshal(event)
+		if err != nil {
+			t.Fatalf("marshal failed: %v", err)
+		}
+
+		var got map[string]interface{}
+		if err := json.Unmarshal(jsonData, &got); err != nil {
+			t.Fatalf("unmarshal failed: %v", err)
+		}
+
+		spans, ok := got["spans"].([]interface{})
+		if !ok || len(spans) != 1 {
+			t.Fatalf("expected one span in JSON, got %s", jsonData)
+		}
+	})
+
 	t.Run("marshaled output matches original event", func(t *testing.T) {
 		event := &Event{
 			EventID: "12345678901234567890123456789012",
This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

@giortzisg
Copy link
Contributor Author

@cursor push 2b9dc34

@giortzisg giortzisg merged commit fcdae70 into master Mar 3, 2026
18 checks passed
@giortzisg giortzisg deleted the fix/safe-serialization branch March 3, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Telemetry Buffer serialization panic

3 participants