feat(promhttp): add CoalesceGather option to deduplicate concurrent Gather calls

[kakkoyun]

What problem does this solve?

Issue #1477 reports apparent goroutine leaks when using prometheus/client_golang. The root cause is not a WaitGroup bug — Registry.Gather() logic is correct. The real problem:

When a collector's Collect() is slower than the scrape interval, each incoming HTTP scrape triggers an independent Gather(). Each call spawns its own goroutine pipeline. With no upper bound, concurrent pipelines grow proportionally to (scrape rate / collection time) — a goroutine pile-up that looks like a leak but is unbounded concurrent work.

This was originally investigated by @initialed85 in this comment, who validated the issue and prototyped a "piggybacking" (singleflight) approach. This PR implements that idea cleanly inside the HTTP handler, with zero new public types and zero overhead when disabled.

Solution

Add HandlerOpts.CoalesceGather bool. When true, HandlerForTransactional wraps the TransactionalGatherer in an unexported coalescingGatherer that allows only one Gather() to run at a time. Concurrent HTTP requests arriving while a gather is in-flight join the existing cycle and receive the same result.

Usage

http.Handle("/metrics", promhttp.HandlerFor(reg, promhttp.HandlerOpts{
    CoalesceGather: true,
}))

Design decisions

Decision	Rationale
HandlerOpts field, not a public wrapper type	Zero new public types; users opt in with one field
Per-cycle gatherCycle object	Prevents a race where a new cycle overwrites result fields while prior waiters are still reading them
Mutex-based ref counting (not atomic)	Ensures c.cycle = nil is cleared before cy.done() is called, ruling out double-done on a stale pointer
close(cy.ready) as the visibility barrier	Go memory model guarantees cy.mfs/err/done are safely readable after <-cy.ready without additional locking
Default false	Zero overhead; identical code path to today when not enabled

Changes

File	Change
prometheus/promhttp/http.go	Add unexported coalescingGatherer + gatherCycle types; add HandlerOpts.CoalesceGather; wire into HandlerForTransactional
prometheus/promhttp/http_test.go	Add 4 tests: sequential invariant, done-called-exactly-once, goroutine-leak-free (goleak), fresh-cycle-after-release

Test plan

• go test -race -count=1 ./prometheus/promhttp/... — all pass, no data races
• go vet ./prometheus/... — clean
• TestCoalesceGatherGoroutineLeakFree uses goleak.VerifyNone to assert no leaked goroutines under concurrent slow-collector load
• TestCoalesceGatherDoneCalledExactlyOnce verifies the TransactionalGatherer contract: done() calls == Gather() calls regardless of concurrency

Closes #1477

[initialed85]

@kakkoyun LGTM- I tested with the roughly same approach as for my hack implementation (ref.: ncabatoff/process-exporter@master...initialed85:process-exporter:kakkoyun/goroutine_leak)

Worked as expected- with a fake 1s sleep added to the scrape method in process-exporter:

• A request loop with a 600ms timeout would would return a result every other request
• 4 x concurrent request loops with no timeout would get a result for every request (after 1s)
• process_open_fds reported by the process-exporter stayed pretty steady at 11 or 12 (impacted by whether or not the fast request loop was active at the time)
• process_open_fds dropped down to 9 if I killed 2 of the 4 concurrent request loops

So I think we're in good shape- lots of timing out of requests, no climbing FDs; works nicely.

Awesome work!

[vesari]

One question: if c.g.Gather() panics, do the joiners actually see the panic, or do they just unblock and return as if everything was ok?

[kakkoyun]

This is a bit of defensive coding here. They just unblock and clean up. If there is no recover in the chain, it will be printed as any other panic.

Feel free to challenge and propose better alternative. Would you prefer us to capture the error?

[vesari]

I made a suggestion above to capture the error. This is because I was thinking that HandlerForTransactional basically returns an http.Handler and if I’m reading this correctly https://github.com/golang/go/blob/e2fa8596cf7016d080c51d772c3012f2533e2219/src/net/http/server.go#L84, then the panics are recovered in any case.

[kakkoyun]

I set a sentinel error (errGatherPanicked) before closing the cycle, so joiners fail with it rather than returning an empty 200. As you spotted, net/http recovers the leader's panic, so no recover is needed here. Added a test.

[vesari]

I left just a small suggestion.

[vesari]

Suggested change

	close(cy.ready)
	cy.err = fmt.Errorf("gather panicked")
	close(cy.ready)

[vesari]

I made a suggestion above to capture the error. This is because I was thinking that HandlerForTransactional basically returns an http.Handler and if I’m reading this correctly https://github.com/golang/go/blob/e2fa8596cf7016d080c51d772c3012f2533e2219/src/net/http/server.go#L84, then the panics are recovered in any case.

[kakkoyun]

@bwplotka Could you have a look at this before the next release?

[Copilot]

Pull request overview

This PR adds an opt-in promhttp.HandlerOpts.CoalesceGather flag to deduplicate concurrent TransactionalGatherer.Gather() calls inside the promhttp HTTP handler, so overlapping scrapes share a single in-flight gather cycle and avoid unbounded concurrent gather pipelines under slow collectors.

Changes:

• Add an unexported coalescingGatherer/gatherCycle implementation to coalesce concurrent Gather() calls behind HandlerForTransactional.
• Add HandlerOpts.CoalesceGather documentation and wiring in HandlerForTransactional.
• Add new promhttp tests intended to validate coalescing behavior and goroutine hygiene.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
prometheus/promhttp/http.go	Adds CoalesceGather option and an internal TransactionalGatherer wrapper that coalesces concurrent gather cycles.
prometheus/promhttp/http_test.go	Adds tests and helpers around coalesced gather behavior, done-callback invariants, and goroutine leak checks.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bwplotka]

I did start looking, I wonder if it shouldn't be a default behaviour 🤔

I also don't get yet the panic behaviour given we don't want directly use recover. I also don't see any test for it at the moment.

[kakkoyun]

Thanks for looking. Two parts.

On default vs opt-in: I'd keep it opt-in. Turning it on by default changes semantics for everyone. Concurrent scrapers would share one snapshot per cycle, and the "returned MetricFamily must not be mutated" rule becomes mandatory, which breaks custom gatherers that mutate families after Gather returns. Opt-in keeps zero overhead and today's exact behaviour when off. If we do want it on by default, I'd rather do that as a separate change with its own discussion.

On the panic behaviour: you're right it wasn't clear. The common case never hits special handling. A panicking Collector is already recovered by the registry (safeCollect) and returned as an error, so joiners get that error like any other. The guard only fires if the wrapped gatherer itself panics. Before, joiners were unblocked with a nil error and returned an empty 200, which is the bug. Now the leader's panic still propagates to net/http (no recover, as you preferred), and joiners get a sentinel error so they fail with 500 instead of an empty body. I added TestCoalesceGatherPanicUnblocksJoinersWithError and expanded the comments. If you'd rather match safeCollect and recover, then wrap the panic into an error for all callers, that's a small change and I'm happy to switch.

[kakkoyun]

Pushed the review follow-ups: joiners now fail with a sentinel error on a gatherer panic instead of an empty 200; the done-once test is deterministic (exactly one Gather/done); and each coalesced caller now gets its own slices.Clone of the metric-family slice, so per-request name[] filtering or reordering can't race another caller sharing the cycle. The CoalesceGather doc now spells out the shared-snapshot staleness, the Timeout interaction, and slice-vs-contents mutation. CI is green.