fix(promhttp): give coalesced callers independent metric slices

Concurrent callers sharing a coalesced Gather cycle previously received the same slice header. A caller that filtered or reordered its slice (for example applying name[] query parameters) could then race another caller of the same cycle. Each caller now gets its own slices.Clone; the MetricFamily values stay shared and read-only.

Also clarify the CoalesceGather documentation (shared-snapshot staleness, the Timeout interaction, and slice-versus-contents mutation) and tidy the sentinel error and coalescer test helpers.

Signed-off-by: Kemal Akkoyun <kemal.akkoyun@datadoghq.com>

Author: kakkoyun

prometheus/promhttp/http.go

@@ -37,6 +37,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"slices"
 	"strconv"
 	"sync"
 	"time"
@@ -96,7 +97,7 @@ type coalescingGatherer struct {
 // gatherCycle tracks a single in-flight Gather and all HTTP handlers sharing it.
 type gatherCycle struct {
 	ready chan struct{}       // closed when Gather completes; happens-before reads of mfs/err/done
-	mfs   []*dto.MetricFamily // set before ready is closed; shared across handlers, must not be mutated
+	mfs   []*dto.MetricFamily // canonical result, set before ready is closed; callers get a slices.Clone, the element values stay shared and must not be mutated
 	err   error               // set before ready is closed
 	done  func()              // underlying done callback; set before ready is closed
 	refs  int                 // number of handlers using this cycle; protected by coalescingGatherer.mu
@@ -107,7 +108,7 @@ var _ prometheus.TransactionalGatherer = (*coalescingGatherer)(nil) // compile-t
 // errGatherPanicked is returned to callers that joined an in-flight coalesced
 // Gather whose underlying gatherer panicked. See the panic guard in Gather for
 // why joiners receive this error instead of the panic itself.
-var errGatherPanicked = errors.New("promhttp: coalesced gather panicked")
+var errGatherPanicked = errors.New("coalesced gather panicked")
 
 func (c *coalescingGatherer) Gather() ([]*dto.MetricFamily, func(), error) {
 	c.mu.Lock()
@@ -116,7 +117,10 @@ func (c *coalescingGatherer) Gather() ([]*dto.MetricFamily, func(), error) {
 		cy.refs++
 		c.mu.Unlock()
 		<-cy.ready
-		return cy.mfs, c.releaseFunc(cy), cy.err
+		// Each caller gets its own slice header so it can filter or reorder
+		// without racing other callers sharing this cycle. The *dto.MetricFamily
+		// values remain shared and must not be mutated in place.
+		return slices.Clone(cy.mfs), c.releaseFunc(cy), cy.err
 	}
 	cy := &gatherCycle{
 		ready: make(chan struct{}),
@@ -136,6 +140,12 @@ func (c *coalescingGatherer) Gather() ([]*dto.MetricFamily, func(), error) {
 	// set cy.err before closing cy.ready so joiners waiting on <-cy.ready fail
 	// with that error instead of silently returning an empty, successful
 	// response, and we clear c.cycle so the next Gather starts a fresh cycle.
+	//
+	// The leader never runs its own releaseFunc on this path, so its ref is
+	// not decremented; that is harmless because the cycle is detached (c.cycle
+	// = nil) and cy.done is still the no-op set at construction (c.g.Gather
+	// panicked before assigning a real done). If cy.done is ever made non-nil
+	// before c.g.Gather runs, this path would need to release it.
 	panicked := true
 	defer func() {
 		if panicked {
@@ -152,7 +162,10 @@ func (c *coalescingGatherer) Gather() ([]*dto.MetricFamily, func(), error) {
 	panicked = false
 	close(cy.ready) // happens-before joiners' reads of cy.mfs/err/done
 
-	return cy.mfs, c.releaseFunc(cy), cy.err
+	// Clone here too so cy.mfs stays the write-once canonical slice: joiners
+	// read it concurrently via slices.Clone, so the leader must not hand out
+	// (and potentially reorder) the same backing array.
+	return slices.Clone(cy.mfs), c.releaseFunc(cy), cy.err
 }
 
 // releaseFunc returns the done callback for one caller sharing cy.
@@ -535,14 +548,23 @@ type HandlerOpts struct {
 	// faster than the time collectors need to produce metrics.
 	//
 	// When enabled, concurrent scrapers share a single metric snapshot per
-	// collection cycle. The returned MetricFamily values are shared and must
-	// not be mutated in place. The built-in handler only reads them, so this
-	// is safe in practice, but custom TransactionalGatherer implementations
-	// that modify the returned families after Gather returns must not use
-	// this option.
+	// collection cycle. Each request receives its own copy of the returned
+	// slice, so filtering or reordering it (for example via name[] query
+	// parameters) is safe. The pointed-to MetricFamily values are still
+	// shared: the built-in handler only reads them, so this is safe in
+	// practice, but a custom TransactionalGatherer that mutates the returned
+	// families in place after Gather returns must not use this option.
+	//
+	// Because the snapshot is shared, a request that arrives while a cycle is
+	// in flight receives that cycle's result even though collection began
+	// before the request; two scrapers joined to one cycle observe the same
+	// timestamps rather than independently gathered data.
 	//
-	// Consider using CoalesceGather together with Timeout to bound both the
-	// scrape response time and the number of concurrent background Gathers.
+	// Consider using CoalesceGather together with Timeout. Timeout bounds the
+	// client-facing response time and keeps at most one collection running at
+	// a time, but it does not cancel the underlying Gather: a joined request
+	// that times out still holds a MaxRequestsInFlight slot until the shared
+	// collection completes.
 	//
 	// Panic handling: a panicking Collector is already turned into an error by
 	// the registry, so joiners receive that error like any other. In the rare

prometheus/promhttp/http_test.go

@@ -1046,13 +1046,18 @@ func (c *coalescingGatherer) inflightRefs() int {
 }
 
 // waitForRefs blocks until cg reports at least want in-flight refs, failing the
-// test if that does not happen promptly.
+// test if that does not happen within a short deadline. It must be called from
+// the test goroutine, as it reports failure via t.Fatalf.
 func waitForRefs(t *testing.T, cg *coalescingGatherer, want int) {
 	t.Helper()
 	deadline := time.Now().Add(2 * time.Second)
-	for cg.inflightRefs() < want {
+	for {
+		got := cg.inflightRefs()
+		if got >= want {
+			return
+		}
 		if time.Now().After(deadline) {
-			t.Fatalf("timed out waiting for %d in-flight refs, have %d", want, cg.inflightRefs())
+			t.Fatalf("timed out waiting for %d in-flight refs, have %d", want, got)
 		}
 		time.Sleep(time.Millisecond)
 	}
@@ -1120,7 +1125,62 @@ func TestCoalesceGatherPanicUnblocksJoinersWithError(t *testing.T) {
 	if res.done != nil {
 		res.done() // must be safe to call: no panic, no double-done.
 	}
+	// inflightRefs reports 0 once c.cycle is detached, which the leader does on
+	// the panic path. This confirms the coalescer starts a fresh cycle next
+	// time; it does not track the orphaned cycle's own ref count (the leader's
+	// ref is intentionally never released there).
 	if got := cg.inflightRefs(); got != 0 {
-		t.Errorf("cycle not cleared after panic: inflightRefs = %d, want 0", got)
+		t.Errorf("cycle not detached after panic: inflightRefs = %d, want 0", got)
+	}
+}
+
+// TestCoalesceGatherCallersGetDistinctSlices verifies that callers sharing one
+// cycle each receive their own slice header, so one caller reordering or
+// filtering its slice cannot race another caller of the same cycle.
+func TestCoalesceGatherCallersGetDistinctSlices(t *testing.T) {
+	defer goleak.VerifyNone(t)
+
+	reg := prometheus.NewRegistry()
+	cnt := prometheus.NewCounter(prometheus.CounterOpts{Name: "c_total", Help: "help"})
+	cnt.Inc()
+	reg.MustRegister(cnt)
+	block := make(chan struct{})
+	started := make(chan struct{}, 1)
+	reg.MustRegister(blockingCollector{CollectStarted: started, Block: block})
+
+	cg := &coalescingGatherer{g: &syncGatherCounter{g: reg}}
+
+	type result struct {
+		mfs  []*dto.MetricFamily
+		done func()
+	}
+	got := make(chan result, 2)
+	call := func() {
+		mfs, done, err := cg.Gather()
+		if err != nil {
+			t.Errorf("Gather returned error: %v", err)
+		}
+		got <- result{mfs: mfs, done: done}
+	}
+
+	go call()
+	<-started             // leader is in-flight, blocked in Collect.
+	go call()             // second caller joins the same cycle.
+	waitForRefs(t, cg, 2) // both callers share the cycle before release.
+	close(block)
+
+	r1 := <-got
+	r2 := <-got
+	defer r1.done()
+	defer r2.done()
+
+	if len(r1.mfs) == 0 || len(r2.mfs) == 0 {
+		t.Fatalf("expected non-empty metric families, got %d and %d", len(r1.mfs), len(r2.mfs))
+	}
+	// Distinct backing arrays: clearing one caller's slot must not affect the
+	// other. This fails if the coalescer hands out the shared slice directly.
+	r1.mfs[0] = nil
+	if r2.mfs[0] == nil {
+		t.Error("callers share a slice backing array; each caller must get its own copy")
 	}
 }