pdh: release partially initialized collectors in the constructors (#2736)

Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>

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.

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(

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
 	}

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

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:"<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`.

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:

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 {

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)