go test -count=2 ./internal/server/... panics on duplicate metrics collector registration #227

New Issue

clawbot · 2026-08-20T06:44:18+02:00

clawbot commented

2026-08-20 06:44:18 +02:00

Found during the review of #224, then reproduced on plain origin/next (bb30b3a) with that branch absent. It is a defect in next, not in any PR.

duplicate metrics collector registration attempted

Path: routes_test.go:734 TestMetricsRouteRequiresCredentials -> middleware.Metrics -> go-http-metrics NewRecorder -> MustRegister (internal/middleware/middleware.go:444). metrics.NewRecorder(metrics.Config{}) leaves Config.Registry defaulted to prometheus.DefaultRegisterer, and MustRegister panics on a duplicate.

TestMetricsRouteRequiresCredentials arrived with #216 — the first internal/server test to construct a metrics-enabled router. One registration happens per test binary today, so the normal gate passes; a second metrics-enabled router test, or -count=2, panics.

Why it is worth fixing rather than tolerating: -count=2 is the ordinary way to smoke out state leaking between runs, and it is currently unavailable for internal/server. It also means any future test that builds a second metrics-enabled router fails for a reason unrelated to what it is testing, which is a trap for whoever writes it.

Remedy (proposed by two independent reviewers): give middleware.New a dedicated prometheus.Registry and serve /metrics through a matching gatherer, instead of the global default registerer. That also decouples the delivery collectors added by #209 from the global default.

Note the delivery collectors in #224 are NOT implicated — they register via sync.OnceValue and pass -count=2.

Definition of done:

go test -count=2 ./internal/server/... passes
two metrics-enabled routers can be constructed in one process without panicking, covered by a test that does exactly that
/metrics still exposes the three existing http_* series and the delivery series from #209
no second /metrics route and no second scrape path

Found during the review of https://git.eeqj.de/sneak/webhooker/pulls/224, then reproduced on plain `origin/next` (`bb30b3a`) with that branch absent. It is a defect in `next`, not in any PR. ``` duplicate metrics collector registration attempted ``` Path: `routes_test.go:734 TestMetricsRouteRequiresCredentials` -> `middleware.Metrics` -> go-http-metrics `NewRecorder` -> `MustRegister` (`internal/middleware/middleware.go:444`). `metrics.NewRecorder(metrics.Config{})` leaves `Config.Registry` defaulted to `prometheus.DefaultRegisterer`, and `MustRegister` panics on a duplicate. `TestMetricsRouteRequiresCredentials` arrived with https://git.eeqj.de/sneak/webhooker/pulls/216 — the first `internal/server` test to construct a metrics-enabled router. One registration happens per test binary today, so the normal gate passes; a second metrics-enabled router test, or `-count=2`, panics. Why it is worth fixing rather than tolerating: `-count=2` is the ordinary way to smoke out state leaking between runs, and it is currently unavailable for `internal/server`. It also means any future test that builds a second metrics-enabled router fails for a reason unrelated to what it is testing, which is a trap for whoever writes it. Remedy (proposed by two independent reviewers): give `middleware.New` a dedicated `prometheus.Registry` and serve `/metrics` through a matching gatherer, instead of the global default registerer. That also decouples the delivery collectors added by https://git.eeqj.de/sneak/webhooker/issues/209 from the global default. Note the delivery collectors in https://git.eeqj.de/sneak/webhooker/pulls/224 are NOT implicated — they register via `sync.OnceValue` and pass `-count=2`. Definition of done: - `go test -count=2 ./internal/server/...` passes - two metrics-enabled routers can be constructed in one process without panicking, covered by a test that does exactly that - `/metrics` still exposes the three existing `http_*` series and the delivery series from https://git.eeqj.de/sneak/webhooker/issues/209 - no second `/metrics` route and no second scrape path

clawbot referenced this issue

2026-08-20 07:09:12 +02:00

Expose delivery metrics on /metrics (closes #209) #224

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#227