From 1838d0964cd5c3909be741f1a1059d88ad905c51 Mon Sep 17 00:00:00 2001
From: Tyler Yahn <MrAlias@users.noreply.github.com>
Date: Thu, 7 Nov 2024 12:43:58 -0800
Subject: [PATCH] Add concurrency testing (#1249)

* Add test-race make target

* Ensure manager is shutdown in test

* Ensure concurrent safe access to noopProbe

* Add race-test GitHub action workflow

* Update .github/workflows/checks.yml

Co-authored-by: Ron Federman <73110295+RonFed@users.noreply.github.com>

---------

Co-authored-by: Ron Federman <73110295+RonFed@users.noreply.github.com>
---
 .github/workflows/checks.yml                 | 15 +++++++++
 Makefile                                     |  3 +-
 internal/pkg/instrumentation/manager_test.go | 33 ++++++++++++++------
 3 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml
index fae363b3a..b738bfe61 100644
--- a/.github/workflows/checks.yml
+++ b/.github/workflows/checks.yml
@@ -54,6 +54,21 @@ jobs:
         run: make license-header-check dependabot-check go-mod-tidy golangci-lint
       - name: Check clean repository
         run: make check-clean-work-tree
+  race-test:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout Repo
+        uses: actions/checkout@v4
+      - name: Setup Go
+        uses: actions/setup-go@v5
+        with:
+          go-version: ${{ matrix.go-version }}
+          check-latest: true
+          cache-dependency-path: "**/go.sum"
+      - name: Install build dependencies
+        run: sudo apt-get update && sudo apt-get install -y clang llvm
+      - name: Run tests
+        run: make test-race
   compatibility-test:
     strategy:
       matrix:
diff --git a/Makefile b/Makefile
index ca5048a36..b2570d821 100644
--- a/Makefile
+++ b/Makefile
@@ -53,10 +53,11 @@ $(TOOLS)/offsetgen: PACKAGE=go.opentelemetry.io/auto/$(TOOLS_MOD_DIR)/inspect/cm
 .PHONY: tools
 tools: $(GOLICENSES) $(MULTIMOD) $(GOLANGCI_LINT) $(DBOTCONF) $(OFFSETGEN)
 
-TEST_TARGETS := test-verbose test-ebpf
+TEST_TARGETS := test-verbose test-ebpf test-race
 .PHONY: $(TEST_TARGETS) test
 test-ebpf: ARGS = -tags=ebpf_test -run ^TestEBPF # These need to be run with sudo.
 test-verbose: ARGS = -v
+test-race: ARGS = -race
 $(TEST_TARGETS): test
 test: go-mod-tidy generate $(ALL_GO_MODS:%=test/%)
 test/%/go.mod:
diff --git a/internal/pkg/instrumentation/manager_test.go b/internal/pkg/instrumentation/manager_test.go
index 1caeabe07..dd21b07b6 100644
--- a/internal/pkg/instrumentation/manager_test.go
+++ b/internal/pkg/instrumentation/manager_test.go
@@ -8,6 +8,7 @@ package instrumentation
 import (
 	"context"
 	"log/slog"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -298,24 +299,24 @@ func (p slowProbe) Close() error {
 }
 
 type noopProbe struct {
-	loaded, running, closed bool
+	loaded, running, closed atomic.Bool
 }
 
 var _ probe.Probe = (*noopProbe)(nil)
 
 func (p *noopProbe) Load(*link.Executable, *process.TargetDetails, *sampling.Config) error {
-	p.loaded = true
+	p.loaded.Store(true)
 	return nil
 }
 
 func (p *noopProbe) Run(c chan<- ptrace.ScopeSpans) {
-	p.running = true
+	p.running.Store(true)
 }
 
 func (p *noopProbe) Close() error {
-	p.closed = true
-	p.loaded = false
-	p.running = false
+	p.closed.Store(true)
+	p.loaded.Store(false)
+	p.running.Store(false)
 	return nil
 }
 
@@ -381,7 +382,19 @@ func TestConfigProvider(t *testing.T) {
 
 	mockExeAndBpffs(t)
 	runCtx, cancel := context.WithCancel(context.Background())
-	go func() { _ = m.Run(runCtx, &process.TargetDetails{PID: 1000}) }()
+	errCh := make(chan error, 1)
+	go func() { errCh <- m.Run(runCtx, &process.TargetDetails{PID: 1000}) }()
+	t.Cleanup(func() {
+		assert.Eventually(t, func() bool {
+			select {
+			case <-errCh:
+				return true
+			default:
+				return false
+			}
+		}, time.Second, 10*time.Millisecond)
+	})
+
 	assert.Eventually(t, func() bool {
 		select {
 		case <-loadedIndicator:
@@ -393,17 +406,17 @@ func TestConfigProvider(t *testing.T) {
 
 	probeRunning := func(id probe.ID) bool {
 		p := m.probes[id].(*noopProbe)
-		return p.loaded && p.running
+		return p.loaded.Load() && p.running.Load()
 	}
 
 	probePending := func(id probe.ID) bool {
 		p := m.probes[id].(*noopProbe)
-		return !p.loaded && !p.running
+		return !p.loaded.Load() && !p.running.Load()
 	}
 
 	probeClosed := func(id probe.ID) bool {
 		p := m.probes[id].(*noopProbe)
-		return p.closed
+		return p.closed.Load()
 	}
 
 	assert.True(t, probePending(netHTTPClientProbeID))