From ca86e22033b3a63d1ea19b25e975118b1d48acf5 Mon Sep 17 00:00:00 2001 From: greymoth-jp Date: Fri, 26 Jun 2026 15:50:04 +0900 Subject: [PATCH 1/2] fix(wrap): recover collector panics in wrappingCollector goroutines PR #1961 added safeCollect to recover panics raised by a collector's Collect method during Registry.Gather, turning a misbehaving collector (issue #1877) into a recoverable error instead of a process crash. However, a collector registered via WrapRegistererWith is wrapped in a wrappingCollector whose Collect (and Describe) runs the inner collector in a NEW goroutine. A recover only catches panics from its own goroutine, so safeCollect in the gather worker cannot catch a panic raised inside the wrappingCollector goroutine; it still crashes the whole process. WrapRegistererWith is the standard way to add constant labels/prefixes, so this leaves the common path unprotected. Recover the panic inside the wrappingCollector goroutines themselves and surface it as an invalid Metric (Collect) / invalid Desc (Describe), mirroring safeCollect's behaviour. Adds regression tests for both paths. Signed-off-by: greymoth-jp --- prometheus/wrap.go | 33 ++++++++++++- prometheus/wrap_panic_probe_test.go | 72 +++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 prometheus/wrap_panic_probe_test.go diff --git a/prometheus/wrap.go b/prometheus/wrap.go index 697f55558..423edfd19 100644 --- a/prometheus/wrap.go +++ b/prometheus/wrap.go @@ -15,6 +15,7 @@ package prometheus import ( "fmt" + "runtime" "sort" "github.com/prometheus/client_golang/prometheus/internal" @@ -160,8 +161,23 @@ type wrappingCollector struct { func (c *wrappingCollector) Collect(ch chan<- Metric) { wrappedCh := make(chan Metric) go func() { + defer close(wrappedCh) + defer func() { + // The wrapped Collect runs in its own goroutine, so a panic + // here cannot be recovered by safeCollect in the Registry (a + // recover only catches panics from the same goroutine). Recover + // it locally and surface it as an invalid metric, mirroring how + // safeCollect handles panics from unwrapped collectors. Without + // this, a single misbehaving collector wrapped via + // WrapRegistererWith crashes the whole process during Gather. + if r := recover(); r != nil { + buf := make([]byte, 64<<10) // 64 KB + n := runtime.Stack(buf, false) + err := fmt.Errorf("prometheus collector panic recovered: type=%T: error=%v\nstack trace=%s", c.wrappedCollector, r, buf[:n]) + wrappedCh <- NewInvalidMetric(NewInvalidDesc(err), err) + } + }() c.wrappedCollector.Collect(wrappedCh) - close(wrappedCh) }() for m := range wrappedCh { ch <- &wrappingMetric{ @@ -175,8 +191,21 @@ func (c *wrappingCollector) Collect(ch chan<- Metric) { func (c *wrappingCollector) Describe(ch chan<- *Desc) { wrappedCh := make(chan *Desc) go func() { + defer close(wrappedCh) + defer func() { + // As in Collect, the wrapped Describe runs in its own goroutine, + // so a panic here would crash the process and cannot be recovered + // by the caller. Recover it locally and surface it as an invalid + // Desc carrying the error, consistent with how invalid + // descriptors are reported elsewhere. + if r := recover(); r != nil { + buf := make([]byte, 64<<10) // 64 KB + n := runtime.Stack(buf, false) + err := fmt.Errorf("prometheus collector panic recovered: type=%T: error=%v\nstack trace=%s", c.wrappedCollector, r, buf[:n]) + wrappedCh <- NewInvalidDesc(err) + } + }() c.wrappedCollector.Describe(wrappedCh) - close(wrappedCh) }() for desc := range wrappedCh { ch <- wrapDesc(desc, c.prefix, c.labels) diff --git a/prometheus/wrap_panic_probe_test.go b/prometheus/wrap_panic_probe_test.go new file mode 100644 index 000000000..f278a385f --- /dev/null +++ b/prometheus/wrap_panic_probe_test.go @@ -0,0 +1,72 @@ +package prometheus + +import ( + "strings" + "testing" +) + +// panicCollector panics in its Collect/Describe methods, simulating a +// misbehaving user collector (the scenario from issue #1877 that PR #1961 set +// out to make non-fatal via safeCollect). +type panicCollector struct { + desc *Desc + panicInCollect bool +} + +func (c *panicCollector) Describe(ch chan<- *Desc) { + if !c.panicInCollect { + panic("boom from wrapped Describe") + } + ch <- c.desc +} + +func (c *panicCollector) Collect(ch chan<- Metric) { + if c.panicInCollect { + panic("boom from wrapped Collect") + } +} + +// TestWrappedCollectorCollectPanicRecovered verifies that a panic raised by a +// collector registered through WrapRegistererWith is recovered during Gather +// (turned into an error) rather than crashing the whole process. +// +// PR #1961 added safeCollect to recover panics from collector.Collect, but a +// wrappingCollector runs the inner collector's Collect in a NEW goroutine, so +// on unfixed code the panic escapes safeCollect (recover only catches panics +// from the same goroutine) and crashes the process. +func TestWrappedCollectorCollectPanicRecovered(t *testing.T) { + reg := NewRegistry() + pc := &panicCollector{desc: NewDesc("probe_metric", "help", nil, nil), panicInCollect: true} + wrapped := WrapRegistererWith(Labels{"zone": "a"}, reg) + wrapped.MustRegister(pc) + + mfs, err := reg.Gather() + if err == nil { + t.Fatalf("expected a recovered-panic error from Gather, got nil (mfs=%d)", len(mfs)) + } + if !strings.Contains(err.Error(), "panic recovered") { + t.Fatalf("expected panic-recovered error, got: %v", err) + } + t.Logf("Collect panic recovered as expected: %v", err) +} + +// TestWrappedCollectorDescribePanicRecovered verifies the symmetric Describe +// path: a panic in a wrapped collector's Describe (e.g. during registration of +// a checked collector) is recovered instead of crashing the process. +func TestWrappedCollectorDescribePanicRecovered(t *testing.T) { + reg := NewRegistry() + pc := &panicCollector{desc: NewDesc("probe_metric", "help", nil, nil), panicInCollect: false} + wrapped := WrapRegistererWith(Labels{"zone": "a"}, reg) + + // Registration triggers Describe on the checked collector. With the fix, + // the panic is recovered and surfaced as a registration error rather than + // crashing the process. + err := wrapped.Register(pc) + if err == nil { + t.Fatalf("expected a recovered-panic error from Register/Describe, got nil") + } + if !strings.Contains(err.Error(), "panic recovered") { + t.Fatalf("expected panic-recovered error, got: %v", err) + } + t.Logf("Describe panic recovered as expected: %v", err) +} From 5facb0d6f0cfa212e1682904ec1d8009f14f3a85 Mon Sep 17 00:00:00 2001 From: greymoth Date: Fri, 26 Jun 2026 22:15:01 +0900 Subject: [PATCH 2/2] chore: add Apache 2.0 license header to wrap_panic_probe_test.go Signed-off-by: greymoth-jp --- prometheus/wrap_panic_probe_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/prometheus/wrap_panic_probe_test.go b/prometheus/wrap_panic_probe_test.go index f278a385f..c0afde618 100644 --- a/prometheus/wrap_panic_probe_test.go +++ b/prometheus/wrap_panic_probe_test.go @@ -1,3 +1,16 @@ +// Copyright The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package prometheus import (