fix(healthcheck): guard target status fields against data race

healthcheck/healthcheck.go

@@ -58,7 +58,13 @@ type Config struct {
 
 // Target represents a health check target with its current status
 type Target struct {
-	Config               Config    `json:"config"`
+	Config Config `json:"config"`
+	// mu guards the mutable status fields below (Status, LastCheck, LastError,
+	// CheckCount, consecutiveSuccesses, consecutiveFailures). They are written by
+	// the target's monitor goroutine and read concurrently when snapshotting for
+	// status reporting. Config and the runtime fields (timer/ctx/cancel/client)
+	// are set once at creation and are not guarded by this mutex.
+	mu                   sync.Mutex
 	Status               Health    `json:"status"`
 	LastCheck            time.Time `json:"lastCheck"`
 	LastError            string    `json:"lastError,omitempty"`
@@ -71,6 +77,22 @@ type Target struct {
 	consecutiveFailures  int
 }
 
+// snapshot returns a copy of the target's reported state taken under the target
+// lock, so status reporting never races with the monitor goroutine's updates.
+// Runtime-only fields (timer/ctx/cancel/client) are intentionally omitted -
+// callers only read the reported status fields.
+func (t *Target) snapshot() *Target {
+	t.mu.Lock()
+	defer t.mu.Unlock()
+	return &Target{
+		Config:     t.Config,
+		Status:     t.Status,
+		LastCheck:  t.LastCheck,
+		LastError:  t.LastError,
+		CheckCount: t.CheckCount,
+	}
+}
+
 // StatusChangeCallback is called when any target's status changes
 type StatusChangeCallback func(targets map[int]*Target)
 
@@ -316,9 +338,8 @@ func (m *Monitor) GetTargets() map[int]*Target {
 func (m *Monitor) getAllTargetsUnsafe() map[int]*Target {
 	targets := make(map[int]*Target)
 	for id, target := range m.targets {
-		// Create a copy to avoid race conditions
-		targetCopy := *target
-		targets[id] = &targetCopy
+		// Snapshot under the target lock to avoid racing with its monitor goroutine
+		targets[id] = target.snapshot()
 	}
 	return targets
 }
@@ -389,12 +410,16 @@ func (m *Monitor) monitorTarget(target *Target) {
 
 // performHealthCheck performs a health check on a target and applies threshold logic
 func (m *Monitor) performHealthCheck(target *Target) {
+	target.mu.Lock()
 	target.CheckCount++
 	target.LastCheck = time.Now()
+	target.mu.Unlock()
 
 	var passed bool
 	var checkErr string
 
+	// Perform the check without holding the target lock so status snapshots are
+	// not blocked for the duration of a (potentially slow) network request.
 	switch strings.ToLower(target.Config.Mode) {
 	case "tcp":
 		passed, checkErr = m.performTCPCheck(target)
@@ -403,6 +428,9 @@ func (m *Monitor) performHealthCheck(target *Target) {
 		passed, checkErr = m.performHTTPCheck(target)
 	}
 
+	target.mu.Lock()
+	defer target.mu.Unlock()
+
 	if passed {
 		target.consecutiveFailures = 0
 		target.consecutiveSuccesses++
@@ -588,7 +616,9 @@ func (m *Monitor) DisableTarget(id int) error {
 		logger.Info("Disabling health check monitoring for target %d", id)
 		target.Config.Enabled = false
 		target.cancel()
+		target.mu.Lock()
 		target.Status = StatusUnknown
+		target.mu.Unlock()
 
 		// Notify callback of status change
 		if m.callback != nil {

healthcheck/healthcheck_test.go

@@ -0,0 +1,83 @@
+package healthcheck
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strconv"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+)
+
+// TestConcurrentStatusSnapshotNoRace exercises the monitor goroutine updating a
+// target's status while status snapshots are taken concurrently (as the status
+// reporter does via GetTargets). It must be run with -race; before the target
+// lock was introduced, getAllTargetsUnsafe copied the whole Target struct while
+// the monitor goroutine mutated it, which the race detector flagged.
+func TestConcurrentStatusSnapshotNoRace(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer srv.Close()
+
+	host, port := splitHostPort(t, srv.URL)
+
+	m := NewMonitor(func(map[int]*Target) {}, false)
+	defer m.Stop()
+
+	cfg := Config{
+		ID: 1, Enabled: true, Mode: "http", Scheme: "http",
+		Hostname: host, Port: port, Path: "/", Status: 200,
+		Interval: 1, Timeout: 2,
+	}
+	if err := m.AddTarget(cfg); err != nil {
+		t.Fatal(err)
+	}
+
+	// Hammer the snapshot path while the monitor goroutine runs its checks.
+	var wg sync.WaitGroup
+	stop := make(chan struct{})
+	for i := 0; i < 4; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for {
+				select {
+				case <-stop:
+					return
+				default:
+					_ = m.GetTargets()
+				}
+			}
+		}()
+	}
+
+	// Force repeated status field writes by re-adding (Replacing) the target,
+	// mirroring the reconnect path that first surfaced the race.
+	for i := 0; i < 20; i++ {
+		_ = m.AddTarget(cfg)
+		time.Sleep(20 * time.Millisecond)
+	}
+
+	close(stop)
+	wg.Wait()
+
+	if got := m.GetTargets(); got[1] == nil {
+		t.Fatal("target 1 missing after churn")
+	}
+}
+
+func splitHostPort(t *testing.T, rawURL string) (string, int) {
+	t.Helper()
+	hp := strings.TrimPrefix(rawURL, "http://")
+	i := strings.LastIndex(hp, ":")
+	if i < 0 {
+		t.Fatalf("no port in %q", rawURL)
+	}
+	port, err := strconv.Atoi(hp[i+1:])
+	if err != nil {
+		t.Fatalf("bad port in %q: %v", rawURL, err)
+	}
+	return hp[:i], port
+}