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..c0afde618 --- /dev/null +++ b/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) +}