fix(metrics): make tracker swap atomic and Enable idempotent for test isolation

internal/metrics/metrics.go

@@ -3,13 +3,17 @@ package metrics
 import (
 	"net/http"
 	"strconv"
+	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 )
 
-type tracker interface {
+// EventTracker is the sink for metric emissions. The process-wide instance is
+// reached through Tracker and swapped with SetTracker.
+type EventTracker interface {
 	TrackRequest(service, method string, status int, duration time.Duration)
 	AddInflightRequest(service string)
 	SubtractInflightRequest(service string)
@@ -27,13 +31,112 @@ type tracker interface {
 	TrackDenial(service, rule string)
 }
 
-var Tracker tracker = &nullTracker{}
+// Tracker delegates every emission to the tracker SetTracker installed last (a
+// no-op until Enable runs). The delegate is behind an atomic pointer because
+// installation must not race with request goroutines emitting through it --
+// and in tests, background work outlives the test that started it.
+var Tracker = &delegatingTracker{}
 
+var enableOnce sync.Once
+
+// Enable installs the Prometheus tracker and returns the handler serving its
+// collectors. Registration happens once per process: the collectors live in
+// the default registry, which panics on re-registration, and a second
+// metrics-enabled server (or `go test -count=2`) must reuse them. Later calls
+// also leave the active tracker alone, so a tracker installed in between (a
+// test fake) keeps receiving events.
 func Enable() http.Handler {
-	Tracker = NewPrometheusTracker()
+	enableOnce.Do(func() {
+		SetTracker(NewPrometheusTracker())
+	})
 	return promhttp.Handler()
 }
 
+// SetTracker atomically installs t as the destination for all metric events
+// and returns the tracker it replaced, so tests can restore it.
+func SetTracker(t EventTracker) EventTracker {
+	previous := Tracker.delegate.Swap(&trackerBox{t})
+	if previous == nil {
+		return &nullTracker{}
+	}
+	return previous.t
+}
+
+// trackerBox keeps the atomic pointer to a single concrete type while the
+// tracked value stays an interface.
+type trackerBox struct{ t EventTracker }
+
+type delegatingTracker struct {
+	delegate atomic.Pointer[trackerBox]
+}
+
+func (d *delegatingTracker) active() EventTracker {
+	if box := d.delegate.Load(); box != nil {
+		return box.t
+	}
+	return nullTracker{}
+}
+
+func (d *delegatingTracker) TrackRequest(service, method string, status int, duration time.Duration) {
+	d.active().TrackRequest(service, method, status, duration)
+}
+
+func (d *delegatingTracker) AddInflightRequest(service string) {
+	d.active().AddInflightRequest(service)
+}
+
+func (d *delegatingTracker) SubtractInflightRequest(service string) {
+	d.active().SubtractInflightRequest(service)
+}
+
+func (d *delegatingTracker) SetCertificateExpiry(domain string, isWildcard bool, expiryTime time.Time) {
+	d.active().SetCertificateExpiry(domain, isWildcard, expiryTime)
+}
+
+func (d *delegatingTracker) IncCertificateRenewals(domain string, success bool) {
+	d.active().IncCertificateRenewals(domain, success)
+}
+
+func (d *delegatingTracker) SetCertificateCount(total, wildcard, http01 int) {
+	d.active().SetCertificateCount(total, wildcard, http01)
+}
+
+func (d *delegatingTracker) TrackCacheEvent(service, result string) {
+	d.active().TrackCacheEvent(service, result)
+}
+
+func (d *delegatingTracker) TrackCacheRefusal(service, reason string) {
+	d.active().TrackCacheRefusal(service, reason)
+}
+
+func (d *delegatingTracker) TrackCacheLease(service, outcome string) {
+	d.active().TrackCacheLease(service, outcome)
+}
+
+func (d *delegatingTracker) TrackCacheLeaseWait(service, outcome string) {
+	d.active().TrackCacheLeaseWait(service, outcome)
+}
+
+func (d *delegatingTracker) TrackCacheEviction(service, state string) {
+	d.active().TrackCacheEviction(service, state)
+}
+
+func (d *delegatingTracker) SetDynamicRedirects(service string, hosts, rules int) {
+	d.active().SetDynamicRedirects(service, hosts, rules)
+}
+
+func (d *delegatingTracker) TrackDynamicRedirectPoll(service, outcome string) {
+	d.active().TrackDynamicRedirectPoll(service, outcome)
+}
+
+func (d *delegatingTracker) TrackDynamicRedirect(service string, status int) {
+	d.active().TrackDynamicRedirect(service, status)
+}
+
+func (d *delegatingTracker) TrackDenial(service, rule string) {
+	d.active().TrackDenial(service, rule)
+}
+
 type nullTracker struct{}
 
 func (nullTracker) TrackRequest(service, method string, status int, dur time.Duration)        {}

internal/metrics/metrics_test.go

@@ -1,11 +1,96 @@
 package metrics
 
 import (
+	"sync"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+// recordingTracker captures cache events; everything else is a no-op.
+type recordingTracker struct {
+	nullTracker
+	mu     sync.Mutex
+	events map[string]int
+}
+
+func (r *recordingTracker) TrackCacheEvent(service, result string) {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	if r.events == nil {
+		r.events = map[string]int{}
+	}
+	r.events[service+":"+result]++
+}
+
+func (r *recordingTracker) eventCount(service, result string) int {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	return r.events[service+":"+result]
+}
+
+func TestSetTracker_DelegatesEventsAndRestores(t *testing.T) {
+	recording := &recordingTracker{}
+	previous := SetTracker(recording)
+	t.Cleanup(func() { SetTracker(previous) })
+
+	Tracker.TrackCacheEvent("service1", "hit")
+	assert.Equal(t, 1, recording.eventCount("service1", "hit"))
+
+	restored := SetTracker(previous)
+	assert.Same(t, recording, restored, "SetTracker returns the tracker it replaced")
+
+	Tracker.TrackCacheEvent("service1", "hit")
+	assert.Equal(t, 1, recording.eventCount("service1", "hit"), "events after restore must not reach the removed tracker")
+}
+
+// Two Enable calls happen whenever a second metrics-enabled Server starts in
+// the same process -- most importantly `go test -count=2`. The second call must
+// reuse the registered collectors instead of panicking on re-registration.
+func TestEnable_IsIdempotent(t *testing.T) {
+	require.NotPanics(t, func() {
+		require.NotNil(t, Enable())
+		require.NotNil(t, Enable())
+	})
+}
+
+// A tracker installed after Enable (a test fake) must survive further Enable
+// calls -- Enable clobbering it is what made cache-metric assertions depend on
+// which tests ran before them.
+func TestEnable_DoesNotReplaceATrackerInstalledAfterIt(t *testing.T) {
+	Enable()
+
+	recording := &recordingTracker{}
+	previous := SetTracker(recording)
+	t.Cleanup(func() { SetTracker(previous) })
+
+	Enable()
+
+	Tracker.TrackCacheEvent("service1", "miss")
+	assert.Equal(t, 1, recording.eventCount("service1", "miss"))
+}
+
+// Swapping the tracker while request goroutines emit through it must be free
+// of data races (run with -race).
+func TestSetTracker_IsSafeUnderConcurrentEmission(t *testing.T) {
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		for range 1000 {
+			Tracker.TrackRequest("service1", "GET", 200, time.Millisecond)
+		}
+	}()
+
+	original := SetTracker(&recordingTracker{})
+	for range 100 {
+		SetTracker(&recordingTracker{})
+	}
+	SetTracker(original)
+	<-done
+}
+
 func TestNormalizeMethod(t *testing.T) {
 	assert.Equal(t, "GET", normalizeMethod("GET"))
 	assert.Equal(t, "POST", normalizeMethod("POST"))

internal/server/cert_metrics_test.go

@@ -4,7 +4,6 @@ import (
 	"strconv"
 	"strings"
 	"sync"
-	"sync/atomic"
 	"testing"
 	"time"
 
@@ -230,112 +229,15 @@ func (f *fakeTracker) renewalCount(domain string, success bool) int {
 	return f.renewals[key]
 }
 
-// switchableTracker is installed into metrics.Tracker exactly once, before any
-// test runs, and thereafter only its delegate changes.
-//
-// Swapping metrics.Tracker itself is a data race: it is a package-level variable
-// that every request path reads, and background work outlives the test that
-// started it -- a stale revalidation goroutine reading the tracker while the
-// next test installs its own is a genuine concurrent write. Swapping an atomic
-// delegate instead removes the write entirely.
-type switchableTracker struct {
-	delegate atomic.Pointer[fakeTracker]
-}
-
-var activeTracker = &switchableTracker{}
-
-func init() {
-	// In init rather than in installFakeTracker: at this point no goroutine
-	// exists that could be reading the variable.
-	metrics.Tracker = activeTracker
-}
-
-func (s *switchableTracker) current() *fakeTracker { return s.delegate.Load() }
-
-func (s *switchableTracker) TrackRequest(service, method string, status int, dur time.Duration) {}
-func (s *switchableTracker) AddInflightRequest(service string)                                  {}
-func (s *switchableTracker) SubtractInflightRequest(service string)                             {}
-
-func (s *switchableTracker) SetCertificateExpiry(domain string, isWildcard bool, expiry time.Time) {
-	if fake := s.current(); fake != nil {
-		fake.SetCertificateExpiry(domain, isWildcard, expiry)
-	}
-}
-
-func (s *switchableTracker) IncCertificateRenewals(domain string, success bool) {
-	if fake := s.current(); fake != nil {
-		fake.IncCertificateRenewals(domain, success)
-	}
-}
-
-func (s *switchableTracker) SetCertificateCount(total, wildcard, http01 int) {
-	if fake := s.current(); fake != nil {
-		fake.SetCertificateCount(total, wildcard, http01)
-	}
-}
-
-func (s *switchableTracker) TrackCacheEvent(service, result string) {
-	if fake := s.current(); fake != nil {
-		fake.TrackCacheEvent(service, result)
-	}
-}
-
-func (s *switchableTracker) TrackCacheRefusal(service, reason string) {
-	if fake := s.current(); fake != nil {
-		fake.TrackCacheRefusal(service, reason)
-	}
-}
-
-func (s *switchableTracker) TrackCacheLease(service, outcome string) {
-	if fake := s.current(); fake != nil {
-		fake.TrackCacheLease(service, outcome)
-	}
-}
-
-func (s *switchableTracker) TrackCacheLeaseWait(service, outcome string) {
-	if fake := s.current(); fake != nil {
-		fake.TrackCacheLeaseWait(service, outcome)
-	}
-}
-
-func (s *switchableTracker) TrackCacheEviction(service, state string) {
-	if fake := s.current(); fake != nil {
-		fake.TrackCacheEviction(service, state)
-	}
-}
-
-func (s *switchableTracker) SetDynamicRedirects(service string, hosts, rules int) {
-	if fake := s.current(); fake != nil {
-		fake.SetDynamicRedirects(service, hosts, rules)
-	}
-}
-
-func (s *switchableTracker) TrackDynamicRedirectPoll(service, outcome string) {
-	if fake := s.current(); fake != nil {
-		fake.TrackDynamicRedirectPoll(service, outcome)
-	}
-}
-
-func (s *switchableTracker) TrackDynamicRedirect(service string, status int) {
-	if fake := s.current(); fake != nil {
-		fake.TrackDynamicRedirect(service, status)
-	}
-}
-
-func (s *switchableTracker) TrackDenial(service, rule string) {
-	if fake := s.current(); fake != nil {
-		fake.TrackDenial(service, rule)
-	}
-}
-
 // installFakeTracker points the tracker at a fresh capturing tracker for the
-// duration of one test.
+// duration of one test. metrics.SetTracker swaps atomically, so background
+// work left over from other tests can keep emitting while it happens.
 func installFakeTracker(t *testing.T) *fakeTracker {
 	t.Helper()
 
 	fake := newFakeTracker()
-	previous := activeTracker.delegate.Swap(fake)
-	t.Cleanup(func() { activeTracker.delegate.Store(previous) })
+	previous := metrics.SetTracker(fake)
+	t.Cleanup(func() { metrics.SetTracker(previous) })
 
 	return fake
 }