diff --git a/healthcheck/healthcheck.go b/healthcheck/healthcheck.go index c0e527b2..51a52020 100644 --- a/healthcheck/healthcheck.go +++ b/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 { diff --git a/healthcheck/healthcheck_test.go b/healthcheck/healthcheck_test.go new file mode 100644 index 00000000..238c30ef --- /dev/null +++ b/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 +}