Recover panics from wrapped collectors (match Gather safeCollect)

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)

prometheus/wrap_panic_probe_test.go

@@ -0,0 +1,85 @@
+// 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 (
+	"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)
+}