fix(promhttp): fail coalesced joiners when the gatherer panics

When CoalesceGather is enabled and the wrapped gatherer itself panics, requests that joined the in-flight cycle were unblocked with a nil error and returned an empty but successful response. They now receive a sentinel error and fail like the panicking request instead of silently reporting no metrics. The leader's panic still propagates and is handled by net/http, matching the non-coalesced path; no recover is introduced.

Also document the panic behaviour on the option, make the coalescing invariant test deterministic instead of timing-dependent, and align the goroutine-leak test with the repository's WaitGroup convention.

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

Author: kakkoyun

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## Unreleased
 
 * [FEATURE] HTTP handlers created by `promhttp` package now support metrics filtering by providing one or more `name[]` query parameters. The default behavior when none are provided remains the same, returning all metrics. #1925
+* [FEATURE] promhttp: Add opt-in `HandlerOpts.CoalesceGather` to deduplicate concurrent `Gather` calls so overlapping scrapes share one collection cycle, preventing goroutine pile-up when the scrape rate outpaces collection time. #1969
 
 ## Unreleased `exp` module
 

prometheus/promhttp/http.go

@@ -104,6 +104,11 @@ type gatherCycle struct {
 
 var _ prometheus.TransactionalGatherer = (*coalescingGatherer)(nil) // compile-time interface check
 
+// 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")
+
 func (c *coalescingGatherer) Gather() ([]*dto.MetricFamily, func(), error) {
 	c.mu.Lock()
 	if cy := c.cycle; cy != nil {
@@ -121,9 +126,16 @@ func (c *coalescingGatherer) Gather() ([]*dto.MetricFamily, func(), error) {
 	c.cycle = cy
 	c.mu.Unlock()
 
-	// Guard against a panic in c.g.Gather: close cy.ready and clear c.cycle
-	// so that any joiners waiting on <-cy.ready are unblocked rather than
-	// deadlocked, and future Gather calls start a fresh cycle.
+	// Guard against a panic in c.g.Gather. The common case, a panicking
+	// Collector, never reaches here: Registry.Gather recovers Collector panics
+	// and returns them as an error. This guard only covers the rare case where
+	// the wrapped gatherer itself panics.
+	//
+	// We deliberately do not recover: the leader's panic propagates and is
+	// handled by net/http exactly as it would be without coalescing. We only
+	// 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.
 	panicked := true
 	defer func() {
 		if panicked {
@@ -132,6 +144,7 @@ func (c *coalescingGatherer) Gather() ([]*dto.MetricFamily, func(), error) {
 				c.cycle = nil
 			}
 			c.mu.Unlock()
+			cy.err = errGatherPanicked // set before close: happens-before joiners' reads
 			close(cy.ready)
 		}
 	}()
@@ -530,6 +543,12 @@ type HandlerOpts struct {
 	//
 	// Consider using CoalesceGather together with Timeout to bound both the
 	// scrape response time and the number of concurrent background Gathers.
+	//
+	// 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
+	// case where the wrapped gatherer itself panics, the panicking request's
+	// panic propagates as usual (handled by net/http), while requests that
+	// joined the same cycle receive an error rather than an empty response.
 	CoalesceGather bool
 	// If handling a request takes longer than Timeout, it is responded to
 	// with 503 ServiceUnavailable and a suitable Message. No timeout is

prometheus/promhttp/http_test.go

@@ -22,7 +22,6 @@ import (
 	"log"
 	"net/http"
 	"net/http/httptest"
-	"runtime"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -920,8 +919,8 @@ func TestCoalesceGatherSequentialInvariant(t *testing.T) {
 	}
 }
 
-// TestCoalesceGatherDoneCalledExactlyOnce verifies that when concurrent requests
-// share a single Gather cycle, the underlying done callback is called exactly once.
+// TestCoalesceGatherDoneCalledExactlyOnce verifies that when a second request
+// joins an in-flight Gather cycle, exactly one Gather and one done call occur.
 func TestCoalesceGatherDoneCalledExactlyOnce(t *testing.T) {
 	defer goleak.VerifyNone(t)
 
@@ -930,8 +929,12 @@ func TestCoalesceGatherDoneCalledExactlyOnce(t *testing.T) {
 	started := make(chan struct{}, 1)
 	reg.MustRegister(blockingCollector{CollectStarted: started, Block: block})
 
+	// Wrap explicitly so the test can observe when request 2 has joined the
+	// in-flight cycle. HandlerForTransactional with CoalesceGather builds an
+	// equivalent wrapper internally but keeps it hidden from the test.
 	counter := &syncGatherCounter{g: reg}
-	handler := HandlerForTransactional(counter, HandlerOpts{CoalesceGather: true})
+	cg := &coalescingGatherer{g: counter}
+	handler := HandlerForTransactional(cg, HandlerOpts{})
 	req, _ := http.NewRequest(http.MethodGet, "/", nil)
 	req.Header.Add(acceptHeader, acceptTextPlain)
 
@@ -952,21 +955,19 @@ func TestCoalesceGatherDoneCalledExactlyOnce(t *testing.T) {
 		close(req2Done)
 	}()
 
-	// Yield to allow request 2 to enter the coalescing wait before releasing.
-	runtime.Gosched()
+	// Wait until request 2 has joined the cycle (refs == 2) before releasing,
+	// so coalescing is guaranteed rather than dependent on goroutine timing.
+	waitForRefs(t, cg, 2)
 	close(block)
 	<-req1Done
 	<-req2Done
 
-	// Key invariant: done() must be called exactly once per Gather cycle.
-	gathers := counter.gatherCalled.Load()
-	dones := counter.doneCalled.Load()
-	if gathers != dones {
-		t.Errorf("Gather called %d times but done called %d times; invariant violated", gathers, dones)
+	// With request 2 joining an in-flight cycle, both share one Gather and one done.
+	if got := counter.gatherCalled.Load(); got != 1 {
+		t.Errorf("Gather called %d times for 2 coalesced requests, want exactly 1", got)
 	}
-	// Coalescing should keep gather count below the number of requests.
-	if gathers > 2 {
-		t.Errorf("Gather called %d times for 2 requests; expected ≤ 2 with coalescing", gathers)
+	if got := counter.doneCalled.Load(); got != 1 {
+		t.Errorf("done called %d times, want exactly 1", got)
 	}
 	for i, w := range []*httptest.ResponseRecorder{w1, w2} {
 		if got, want := w.Code, http.StatusOK; got != want {
@@ -994,9 +995,11 @@ func TestCoalesceGatherGoroutineLeakFree(t *testing.T) {
 
 	var wg sync.WaitGroup
 	for range 5 {
-		wg.Go(func() {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
 			handler.ServeHTTP(httptest.NewRecorder(), req)
-		})
+		}()
 	}
 	<-started
 	close(block)
@@ -1031,3 +1034,95 @@ func TestCoalesceGatherNewCycleAfterCompletion(t *testing.T) {
 		t.Errorf("after cycle 2: done called %d times, want %d", got, want)
 	}
 }
+
+// inflightRefs reports how many callers currently share the in-flight cycle.
+// It lives in the test file so production code carries no test-only hooks; tests
+// use it to wait deterministically until a joiner has entered the cycle.
+func (c *coalescingGatherer) inflightRefs() int {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	if c.cycle == nil {
+		return 0
+	}
+	return c.cycle.refs
+}
+
+// waitForRefs blocks until cg reports at least want in-flight refs, failing the
+// test if that does not happen promptly.
+func waitForRefs(t *testing.T, cg *coalescingGatherer, want int) {
+	t.Helper()
+	deadline := time.Now().Add(2 * time.Second)
+	for cg.inflightRefs() < want {
+		if time.Now().After(deadline) {
+			t.Fatalf("timed out waiting for %d in-flight refs, have %d", want, cg.inflightRefs())
+		}
+		time.Sleep(time.Millisecond)
+	}
+}
+
+// panicGatherer is a TransactionalGatherer whose Gather panics after signaling
+// that it started and waiting to be released. A panicking Collector cannot reach
+// the panic guard in coalescingGatherer because Registry.Gather recovers
+// Collector panics into errors, so the panic is raised at the gatherer level.
+type panicGatherer struct {
+	started chan struct{}
+	block   chan struct{}
+}
+
+func (p *panicGatherer) Gather() ([]*dto.MetricFamily, func(), error) {
+	close(p.started)
+	<-p.block
+	panic("boom")
+}
+
+// TestCoalesceGatherPanicUnblocksJoinersWithError verifies that when the wrapped
+// gatherer panics, a request that joined the same cycle receives errGatherPanicked
+// instead of a nil error with an empty response, the leader's panic still
+// propagates, and the cycle is cleared afterward.
+func TestCoalesceGatherPanicUnblocksJoinersWithError(t *testing.T) {
+	defer goleak.VerifyNone(t)
+
+	p := &panicGatherer{started: make(chan struct{}), block: make(chan struct{})}
+	cg := &coalescingGatherer{g: p}
+
+	// Leader creates the cycle and panics once released; recover so the test
+	// goroutine survives, mirroring net/http's per-request panic recovery.
+	leaderPanicked := make(chan any, 1)
+	go func() {
+		defer func() { leaderPanicked <- recover() }()
+		_, done, _ := cg.Gather()
+		if done != nil {
+			done()
+		}
+	}()
+	<-p.started
+
+	// Joiner joins the in-flight cycle and blocks on <-cy.ready.
+	type joinResult struct {
+		done func()
+		err  error
+	}
+	joined := make(chan joinResult, 1)
+	go func() {
+		_, done, err := cg.Gather()
+		joined <- joinResult{done: done, err: err}
+	}()
+
+	// Ensure the joiner has joined the cycle before releasing the panic.
+	waitForRefs(t, cg, 2)
+	close(p.block)
+
+	if r := <-leaderPanicked; r == nil {
+		t.Error("leader Gather did not panic; want the panic to propagate")
+	}
+	res := <-joined
+	if !errors.Is(res.err, errGatherPanicked) {
+		t.Errorf("joiner err = %v, want errGatherPanicked", res.err)
+	}
+	if res.done != nil {
+		res.done() // must be safe to call: no panic, no double-done.
+	}
+	if got := cg.inflightRefs(); got != 0 {
+		t.Errorf("cycle not cleared after panic: inflightRefs = %d, want 0", got)
+	}
+}