feat(promhttp): add CoalesceGather option to deduplicate concurrent Gather calls by kakkoyun · Pull Request #1969 · prometheus/client_golang

kakkoyun · 2026-03-25T09:51:21Z

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 · 2026-03-30T01:56:07Z

@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 · 2026-04-13T08:44:05Z

+			close(cy.ready)
+		}
+	}()
+	cy.mfs, cy.done, cy.err = c.g.Gather()


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?

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?

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.

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 · 2026-04-20T08:42:40Z

+				c.cycle = nil
+			}
+			c.mu.Unlock()
+			close(cy.ready)


Suggested change

close(cy.ready)

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

close(cy.ready)

vesari · 2026-04-20T08:47:34Z

+			close(cy.ready)
+		}
+	}()
+	cy.mfs, cy.done, cy.err = c.g.Gather()


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 · 2026-07-02T11:24:47Z

@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 · 2026-07-02T13:46:45Z

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 · 2026-07-07T09:02:09Z

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 · 2026-07-07T09:02:11Z

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.

…ather calls When a collector's Collect() is slower than the scrape interval, each incoming HTTP request triggers an independent Gather() that spawns its own goroutine pipeline. With no upper bound, this causes goroutine pile-up proportional to (scrape rate / collection time) — the apparent "goroutine leak" reported in prometheus#1477. Add HandlerOpts.CoalesceGather bool. When true, HandlerForTransactional wraps the underlying TransactionalGatherer in a coalescingGatherer that allows only one Gather to run at a time. Concurrent requests join the in-flight cycle and receive the same result once it completes. Reference counting ensures the TransactionalGatherer done() callback is called exactly once, after the last handler finishes encoding. Design decisions: - Per-cycle gatherCycle object (not fields on the struct) prevents the 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) happens-before <-cy.ready returns (Go memory model), so cy.mfs/err/done are safely readable without additional locking. - Zero overhead when disabled: single if-branch in HandlerForTransactional setup, identical code path when CoalesceGather is false. Fixes prometheus#1477 Signed-off-by: Kemal Akkoyun <kemal.akkoyun@datadoghq.com>

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>

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>

kakkoyun mentioned this pull request Mar 25, 2026

A potential goroutine memory leak #1477

Closed

kakkoyun force-pushed the kakkoyun/goroutine_leak branch 2 times, most recently from f9e46a2 to 81c415a Compare March 25, 2026 10:50

kakkoyun marked this pull request as ready for review April 7, 2026 10:26

kakkoyun requested review from bwplotka and vesari as code owners April 7, 2026 10:26

vesari reviewed Apr 13, 2026

View reviewed changes

vesari approved these changes Apr 20, 2026

View reviewed changes

coderabbitai Bot mentioned this pull request Jun 4, 2026

feat(exporter): emit user_threshold for tenant custom alerts (ADR-024 B, #741 S3a) vencil/Dynamic-Alerting-Integrations#748

Merged

bwplotka requested a review from Copilot July 2, 2026 12:00

Copilot started reviewing on behalf of bwplotka July 2, 2026 12:01 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

Comment thread prometheus/promhttp/http.go Outdated

Comment thread prometheus/promhttp/http_test.go Outdated

Comment thread prometheus/promhttp/http_test.go

kakkoyun force-pushed the kakkoyun/goroutine_leak branch 2 times, most recently from ea472d7 to ad397ed Compare July 6, 2026 14:29

kakkoyun added 3 commits July 7, 2026 17:32

kakkoyun force-pushed the kakkoyun/goroutine_leak branch from 2dccab5 to 71e5c81 Compare July 8, 2026 12:53

Merge branch 'main' into kakkoyun/goroutine_leak

4f8159a

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

Uh oh!

Conversation

kakkoyun commented Mar 25, 2026

What problem does this solve?

Solution

Usage

Design decisions

Changes

Test plan

Uh oh!

initialed85 commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vesari Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

kakkoyun Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

vesari Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

kakkoyun Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

vesari left a comment

Choose a reason for hiding this comment

Uh oh!

vesari Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

vesari Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

kakkoyun commented Jul 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwplotka commented Jul 2, 2026

Uh oh!

kakkoyun commented Jul 7, 2026

Uh oh!

kakkoyun commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

initialed85 commented Mar 30, 2026 •

edited

Loading