From 126b655afa63c428e07896251e2bb70ba8e68080 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan-Otto=20Kr=C3=B6pke?= Date: Fri, 16 Oct 2026 09:11:11 +0200 Subject: [PATCH] pdh: release partially initialized collectors in the constructors (#2736) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jan-Otto Kröpke --- AGENTS.md | 8 +- .../hyperv/hyperv_virtual_hard_disk.go | 3 - .../collector/netframework/netframework.go | 10 +- .../terminal_services/terminal_services.go | 5 - internal/pdh/AGENTS.md | 38 ++++++ internal/pdh/collector.go | 86 ++++++------ internal/pdh/collector_test.go | 126 +++++++++++++++++- internal/pdh/pdh.go | 34 ++++- 8 files changed, 241 insertions(+), 69 deletions(-) create mode 100644 internal/pdh/AGENTS.md diff --git a/AGENTS.md b/AGENTS.md index 164b1be3..b10afaf0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -140,9 +140,11 @@ exercised a live Windows role. `Commit` or `Reset` in collectors. A direct `Collect` call, as in most tests, runs without the caches; use `testutils.Collect` or `scrapecache.Begin` to test the cached path. -- Release resources after partial initialization and on error paths. A non-nil - PDH collector returned with an error still belongs to the caller and must be - closed. Sub-collector cleanup closures must read the initialized field when +- Release resources after partial initialization and on error paths. PDH + constructors release what they opened and return a nil collector or query + with an error, so close only what was returned without one; `Close` is + nil-safe (see [`internal/pdh/AGENTS.md`](internal/pdh/AGENTS.md)). + Sub-collector cleanup closures must read the initialized field when called; method values captured before initialization can retain nil receivers. Stop background workers before releasing the resources they use, and recover panics inside the goroutine that can panic. diff --git a/internal/collector/hyperv/hyperv_virtual_hard_disk.go b/internal/collector/hyperv/hyperv_virtual_hard_disk.go index f9b6521b..7658de67 100644 --- a/internal/collector/hyperv/hyperv_virtual_hard_disk.go +++ b/internal/collector/hyperv/hyperv_virtual_hard_disk.go @@ -150,9 +150,6 @@ func (c *Collector) buildVirtualHardDisk() error { c.logger.Warn("Hyper-V Virtual Storage Device counters are not available, virtual hard disks have no device label", slog.Any("err", err), ) - - c.virtualHardDiskDevices.Close() - c.virtualHardDiskDevices = nil } c.virtualHardDiskRefresher = newVirtualHardDiskRefresher( diff --git a/internal/collector/netframework/netframework.go b/internal/collector/netframework/netframework.go index a0093538..db1149d5 100644 --- a/internal/collector/netframework/netframework.go +++ b/internal/collector/netframework/netframework.go @@ -380,19 +380,15 @@ func (c *Collector) Collect(ch chan<- prometheus.Metric, maxScrapeDuration time. return g.Wait() } -// newPerfCollector adds the counters of object to query and retains -// ownership of partially initialized collectors. Missing objects are optional; -// missing individual counters remain errors. +// newPerfCollector adds the counters of object to query. On error, +// pdh.NewQueryCollector has already removed the counters it added. Missing +// objects are optional; missing individual counters remain errors. func newPerfCollector[T any](logger *slog.Logger, query *pdh.Query, object string) (perfCollector[T], error) { c, err := pdh.NewQueryCollector[T](logger, query, pdh.CounterTypeRaw, object, pdh.InstancesAll) if err == nil { return c, nil } - if c != nil { - c.Close() - } - if onlyMissingObject(err) { return unavailablePerfCollector[T]{}, nil } diff --git a/internal/collector/terminal_services/terminal_services.go b/internal/collector/terminal_services/terminal_services.go index f9edbef4..14498327 100644 --- a/internal/collector/terminal_services/terminal_services.go +++ b/internal/collector/terminal_services/terminal_services.go @@ -281,8 +281,6 @@ func (c *Collector) Build(logger *slog.Logger, miSession *mi.Session) error { slog.Any("err", err), ) - c.perfDataCollectorBroker.Close() - c.perfDataCollectorBroker = nil c.connectionBrokerEnabled = false } } else { @@ -307,9 +305,6 @@ func (c *Collector) Build(logger *slog.Logger, miSession *mi.Session) error { c.logger.Warn("failed to create Terminal Services Session collector, skipping per-session performance metrics", slog.Any("err", err), ) - - c.perfDataCollectorTerminalServicesSession.Close() - c.perfDataCollectorTerminalServicesSession = nil } return nil diff --git a/internal/pdh/AGENTS.md b/internal/pdh/AGENTS.md new file mode 100644 index 00000000..f9576a97 --- /dev/null +++ b/internal/pdh/AGENTS.md @@ -0,0 +1,38 @@ +# PDH package + +`internal/pdh` wraps the Performance Data Helper API for the typed collectors +(`NewCollector`, `NewQueryCollector`) and the configurable `performancecounter` +collector (`NewDynamicCollector`). + +## Ownership + +- Constructors return either a value and a nil error, or nil and an error. + `NewCollector` and `NewDynamicCollector` close the query they opened, and + `NewQueryCollector` removes the counters it added to the shared query, + before they return an error. Callers never close a value they did not + receive; `Collector.Close` and `Query.Close` accept nil receivers. +- Collectors usually assign the result to a field before checking the error, + so after an error the field is nil. A `Build` that tolerates the error must + check the field for nil in `Collect`; the parent `Close` can close the field + unconditionally. +- `newCollector` holds the lock of a shared query while it adds counters, so + its error paths use `Collector.release`, which expects the caller to hold + the locks or be the only user. `Close` takes the locks and calls it. +- Close collectors of a `Query` before the query itself. + +## Struct tags + +- `perfdata:"Counter"` marks a `float64` field; `perfdata:"Counter,secondvalue"` + receives the second raw value (the base of a ratio or average). +- `perfdata_min_build:""` skips a counter that is missing on an older + Windows build. The value must be a decimal build number that fits a + `uint16`; anything else is a constructor error, so a typo cannot silently + disable the check. + +## Native calls + +- The wrappers return PDH status codes. A path that cannot be converted to + UTF-16 (it contains a NUL character) returns `CstatusBadCountername` + instead of passing a NULL path to PDH. +- Build a `pdh.Error` with `NewPdhError` only for a status that is reported as + an error: it formats the message text with `FormatMessage`. diff --git a/internal/pdh/collector.go b/internal/pdh/collector.go index 06836cf4..7d40a81e 100644 --- a/internal/pdh/collector.go +++ b/internal/pdh/collector.go @@ -91,13 +91,14 @@ type counterField struct { tag string // index is the byte offset of the field in a struct row, or the index // in Row.Values. - index int - minOSBuild string + index int + // minOSBuild is the Windows build that introduced the counter, or 0. A + // counter that is missing on an older build is skipped. + minOSBuild uint16 } type Counter struct { Name string - Desc string // MetricType is the Prometheus metric type of the counter. It is zero // until the counter info of one of its instances has been read. MetricType prometheus.ValueType @@ -118,9 +119,10 @@ type Counter struct { // An instance is only collected if every counter has a valid value for it in that // sample. Use [Collector.MetricType] for the metric type of a counter. // -// If an error is returned together with a non-nil Collector, some counters -// could not be added or the initial collection failed. The caller owns that -// Collector and must Close it. If the Collector is nil, no resources are held. +// NewCollector returns either a Collector and a nil error, or a nil Collector +// and an error. If some counters cannot be added or the initial collection +// fails, NewCollector releases the query and the counters it opened before it +// returns the error, so the caller only closes a Collector it received. func NewCollector[T any](logger *slog.Logger, resultType CounterType, object string, instances []string) (*Collector[T], error) { return newStructCollector[T](logger, nil, resultType, object, instances) } @@ -128,7 +130,8 @@ func NewCollector[T any](logger *slog.Logger, resultType CounterType, object str // NewQueryCollector creates a collector like [NewCollector] whose counters are // part of query. [Query.Collect] samples them together with the other // collectors of the query; [Collector.Collect] reads the latest sample. Close -// the collector before the query. +// the collector before the query. On error, the counters it added are removed +// from query again and the Collector is nil. func NewQueryCollector[T any](logger *slog.Logger, query *Query, resultType CounterType, object string, instances []string) (*Collector[T], error) { if query == nil { return nil, ErrPerformanceCounterNotInitialized @@ -180,6 +183,19 @@ func structFields[T any]() ([]counterField, rowAccessor[T], []error, error) { continue } + var minOSBuild uint16 + + if tag, ok := f.Tag.Lookup("perfdata_min_build"); ok { + build, err := strconv.ParseUint(tag, 10, 16) + if err != nil { + errs = append(errs, fmt.Errorf("field %s: invalid perfdata_min_build %q: %w", f.Name, tag, err)) + + continue + } + + minOSBuild = uint16(build) + } + offset, ok := fieldOffset(valueType, f.Index) if !ok { errs = append(errs, fmt.Errorf("field %s must not be promoted through an embedded pointer", f.Name)) @@ -190,7 +206,7 @@ func structFields[T any]() ([]counterField, rowAccessor[T], []error, error) { fields = append(fields, counterField{ tag: counterName, index: offset, - minOSBuild: f.Tag.Get("perfdata_min_build"), + minOSBuild: minOSBuild, }) } @@ -393,16 +409,9 @@ func newCollector[T any](logger *slog.Logger, query *Query, resultType CounterTy var counterHandle pdhCounterHandle - //nolint:nestif if ret := addEnglishCounter(handle, object, instance, counterName, &counterHandle); ret != ErrorSuccess { - if ret == CstatusNoCounter { - if f.minOSBuild != "" { - if minOSBuild, err := strconv.Atoi(f.minOSBuild); err == nil { - if uint16(minOSBuild) > osversion.Build() { - continue - } - } - } + if ret == CstatusNoCounter && f.minOSBuild > osversion.Build() { + continue } errs = append(errs, fmt.Errorf("failed to add counter %s: %w", counterPath, NewPdhError(ret))) @@ -466,11 +475,7 @@ func newCollector[T any](logger *slog.Logger, query *Query, resultType CounterTy } if len(collector.counters) == 0 { - // A shared query stays open for its other collectors, and its lock - // is held here, so only close a query of our own. - if query == nil { - collector.Close() - } + collector.release() errs = append(errs, errors.New("no counters configured")) @@ -478,7 +483,9 @@ func newCollector[T any](logger *slog.Logger, query *Query, resultType CounterTy } if err := errors.Join(errs...); err != nil { - return collector, fmt.Errorf("failed to initialize collector: %w", err) + collector.release() + + return nil, fmt.Errorf("failed to initialize collector: %w", err) } collector.state = &collectState{} @@ -486,7 +493,9 @@ func newCollector[T any](logger *slog.Logger, query *Query, resultType CounterTy // Collect initial data because some counters need to be read twice to get the correct value. if query != nil { if ret := CollectQueryData(handle); ret != ErrorSuccess && ret != NoData { - return collector, fmt.Errorf("failed to collect initial data: %w", NewPdhError(ret)) + collector.release() + + return nil, fmt.Errorf("failed to collect initial data: %w", NewPdhError(ret)) } return collector, nil @@ -494,7 +503,10 @@ func newCollector[T any](logger *slog.Logger, query *Query, resultType CounterTy var collectValues []T if err := collector.Collect(&collectValues); err != nil && !errors.Is(err, ErrNoData) { - return collector, fmt.Errorf("failed to collect initial data: %w", err) + // Collect has returned, and no one else has the collector yet. + collector.release() + + return nil, fmt.Errorf("failed to collect initial data: %w", err) } return collector, nil @@ -512,23 +524,6 @@ func metricType(resultType CounterType, counterType uint32) prometheus.ValueType return prometheus.GaugeValue } -func (c *Collector[T]) Describe() map[string]string { - if c == nil { - return map[string]string{} - } - - c.mu.RLock() - defer c.mu.RUnlock() - - desc := make(map[string]string, len(c.counters)) - - for _, counter := range c.counters { - desc[counter.Name] = counter.Desc - } - - return desc -} - // MetricType returns the Prometheus metric type of the counter named // counterName, derived from its PDH counter type. Formatted values are always // gauges. It returns false if the counter is unknown or none of its instances @@ -1057,6 +1052,13 @@ func (c *Collector[T]) Close() { c.mu.Lock() defer c.mu.Unlock() + c.release() +} + +// release closes the query of the collector, or removes its counters from a +// shared query, and leaves the collector closed. The caller holds the lock of +// a shared query and c.mu, or is the only user of c, as newCollector is. +func (c *Collector[T]) release() { switch { case c.handle == 0: case c.query == nil: diff --git a/internal/pdh/collector_test.go b/internal/pdh/collector_test.go index 63b3653f..367fd115 100644 --- a/internal/pdh/collector_test.go +++ b/internal/pdh/collector_test.go @@ -170,15 +170,135 @@ func TestCollectorInitializationError(t *testing.T) { HandleCount string `perfdata:"Handle Count"` } + // NewCollector releases what it opened and returns no collector with an error. collector, err := pdh.NewCollector[invalidProcess](slog.New(slog.DiscardHandler), pdh.CounterTypeRaw, "Process", pdh.InstancesAll) require.ErrorContains(t, err, "field HandleCount must be a float64") - require.NotNil(t, collector) - - t.Cleanup(collector.Close) + require.Nil(t, collector) var data []invalidProcess require.ErrorIs(t, collector.Collect(&data), pdh.ErrPerformanceCounterNotInitialized) + + // Closing a nil collector is a no-op. + collector.Close() +} + +// TestCollectorInitializationErrors checks that every constructor error +// returns a nil collector. +func TestCollectorInitializationErrors(t *testing.T) { + t.Parallel() + + logger := slog.New(slog.DiscardHandler) + + t.Run("missing counter", func(t *testing.T) { + t.Parallel() + + type missingCounter struct { + ThreadCount float64 `perfdata:"Thread Count"` + Missing float64 `perfdata:"No Such Counter"` + } + + collector, err := pdh.NewCollector[missingCounter](logger, pdh.CounterTypeRaw, "Process", pdh.InstancesAll) + require.ErrorIs(t, err, pdh.NewPdhError(pdh.CstatusNoCounter)) + require.Nil(t, collector) + }) + + t.Run("missing object", func(t *testing.T) { + t.Parallel() + + collector, err := pdh.NewDynamicCollector(logger, pdh.CounterTypeRaw, "No Such Object", pdh.InstancesAll, []string{"Thread Count"}) + require.ErrorIs(t, err, pdh.NewPdhError(pdh.CstatusNoObject)) + require.Nil(t, collector) + }) + + t.Run("NUL in counter path", func(t *testing.T) { + t.Parallel() + + collector, err := pdh.NewDynamicCollector(logger, pdh.CounterTypeRaw, "Process", pdh.InstancesAll, []string{"Thread\x00Count"}) + require.ErrorIs(t, err, pdh.NewPdhError(pdh.CstatusBadCountername)) + require.Nil(t, collector) + }) + + t.Run("invalid perfdata_min_build", func(t *testing.T) { + t.Parallel() + + type notANumber struct { + ThreadCount float64 `perfdata:"Thread Count" perfdata_min_build:"abc"` + } + + type tooLarge struct { + // Truncated to uint16, this was build 0 and the counter was never skipped. + ThreadCount float64 `perfdata:"Thread Count" perfdata_min_build:"65536"` + } + + type negative struct { + ThreadCount float64 `perfdata:"Thread Count" perfdata_min_build:"-1"` + } + + _, err := pdh.NewCollector[notANumber](logger, pdh.CounterTypeRaw, "Process", pdh.InstancesAll) + require.ErrorContains(t, err, `field ThreadCount: invalid perfdata_min_build "abc"`) + + _, err = pdh.NewCollector[tooLarge](logger, pdh.CounterTypeRaw, "Process", pdh.InstancesAll) + require.ErrorContains(t, err, `field ThreadCount: invalid perfdata_min_build "65536"`) + + collector, err := pdh.NewCollector[negative](logger, pdh.CounterTypeRaw, "Process", pdh.InstancesAll) + require.ErrorContains(t, err, `field ThreadCount: invalid perfdata_min_build "-1"`) + require.Nil(t, collector) + }) + + t.Run("future counter is skipped", func(t *testing.T) { + t.Parallel() + + type futureCounter struct { + ThreadCount float64 `perfdata:"Thread Count"` + Future float64 `perfdata:"No Such Counter" perfdata_min_build:"65535"` + } + + collector, err := pdh.NewCollector[futureCounter](logger, pdh.CounterTypeRaw, "Process", pdh.InstancesAll) + require.NoError(t, err) + + t.Cleanup(collector.Close) + }) +} + +// TestQueryCollectorInitializationError checks that a failing collector +// removes its counters from a shared query and leaves the other collectors of +// the query working. +func TestQueryCollectorInitializationError(t *testing.T) { + t.Parallel() + + logger := slog.New(slog.DiscardHandler) + + query, err := pdh.NewQuery() + require.NoError(t, err) + + t.Cleanup(query.Close) + + type system struct { + Processes float64 `perfdata:"Processes"` + } + + type missingCounter struct { + Processes float64 `perfdata:"Processes"` + Missing float64 `perfdata:"No Such Counter"` + } + + collector, err := pdh.NewQueryCollector[system](logger, query, pdh.CounterTypeRaw, "System", nil) + require.NoError(t, err) + + t.Cleanup(collector.Close) + + failed, err := pdh.NewQueryCollector[missingCounter](logger, query, pdh.CounterTypeRaw, "System", nil) + require.ErrorIs(t, err, pdh.NewPdhError(pdh.CstatusNoCounter)) + require.Nil(t, failed) + + require.NoError(t, query.Collect()) + + var data []system + + require.NoError(t, collector.Collect(&data)) + require.Len(t, data, 1) + require.Positive(t, data[0].Processes) } type processorInformation struct { diff --git a/internal/pdh/pdh.go b/internal/pdh/pdh.go index 895a031d..b97cb589 100644 --- a/internal/pdh/pdh.go +++ b/internal/pdh/pdh.go @@ -364,8 +364,14 @@ func Load() error { // The typeperf command may also be pretty easy. To find all performance counters, simply execute: // // typeperf -qx +// +// A path that contains a NUL character returns CstatusBadCountername. func AddCounter(hQuery pdhQueryHandle, szFullCounterPath string, dwUserData uintptr, phCounter *pdhCounterHandle) uint32 { - ptxt, _ := windows.UTF16PtrFromString(szFullCounterPath) + ptxt, err := windows.UTF16PtrFromString(szFullCounterPath) + if err != nil { + return CstatusBadCountername + } + ret, _, _ := pdhAddCounterW.Call( uintptr(hQuery), uintptr(unsafe.Pointer(ptxt)), @@ -376,13 +382,18 @@ func AddCounter(hQuery pdhQueryHandle, szFullCounterPath string, dwUserData uint } // AddEnglishCounter adds the specified language-neutral counter to the query. See the AddCounter function. This function only exists on -// Windows versions higher than Vista. +// Windows versions higher than Vista. A path that contains a NUL character +// returns CstatusBadCountername. func AddEnglishCounter(hQuery pdhQueryHandle, szFullCounterPath string, dwUserData uintptr, phCounter *pdhCounterHandle) uint32 { if pdhAddEnglishCounterW == nil { return ErrorInvalidFunction } - ptxt, _ := windows.UTF16PtrFromString(szFullCounterPath) + ptxt, err := windows.UTF16PtrFromString(szFullCounterPath) + if err != nil { + return CstatusBadCountername + } + ret, _, _ := pdhAddEnglishCounterW.Call( uintptr(hQuery), uintptr(unsafe.Pointer(ptxt)), @@ -567,8 +578,14 @@ func OpenQuery(szDataSource uintptr, dwUserData uintptr, phQuery *pdhQueryHandle // If a wildcard character is specified in the counter name, all counters of the specified object are returned. // // Partial counter path string matches (for example, "pro*") are supported. +// +// A path that contains a NUL character returns CstatusBadCountername. func ExpandWildCardPath(szWildCardPath string, mszExpandedPathList *uint16, pcchPathListLength *uint32) uint32 { - ptxt, _ := windows.UTF16PtrFromString(szWildCardPath) + ptxt, err := windows.UTF16PtrFromString(szWildCardPath) + if err != nil { + return CstatusBadCountername + } + flags := uint32(0) // expand instances and counters ret, _, _ := pdhExpandWildCardPathW.Call( 0, // search counters on local computer @@ -580,9 +597,14 @@ func ExpandWildCardPath(szWildCardPath string, mszExpandedPathList *uint16, pcch return uint32(ret) } -// ValidatePath validates a path. Will return ErrorSuccess when ok, or PdhCstatusBadCountername when the path is erroneous. +// ValidatePath validates a path. Will return ErrorSuccess when ok, or PdhCstatusBadCountername when the path is erroneous, +// e.g. because it contains a NUL character. func ValidatePath(path string) uint32 { - ptxt, _ := windows.UTF16PtrFromString(path) + ptxt, err := windows.UTF16PtrFromString(path) + if err != nil { + return CstatusBadCountername + } + ret, _, _ := pdhValidatePathW.Call(uintptr(unsafe.Pointer(ptxt))) return uint32(ret)