Share a single vcgencmd runner between temperature and throttled collectors

internal/collector/collector.go

@@ -131,6 +131,10 @@ func New(cfg Config, log *slog.Logger) *Collector {
 		alerts = alert.New(cfg.Thresholds, cfg.AlertFor)
 		notifier = cfg.Notifier
 	}
+	// TemperatureCollector and ThrottledCollector both shell out to
+	// vcgencmd; sharing one runner means vcgencmd is detected once instead
+	// of once per collector.
+	vcg := newVcgencmdRunner(time.Now)
 	c := &Collector{
 		cfg: cfg,
 		// Disks starts as [] rather than nil so it marshals as [] (not
@@ -144,8 +148,8 @@ func New(cfg Config, log *slog.Logger) *Collector {
 		memory:    NewMemoryCollector(),
 		disk:      NewDiskCollector(),
 		network:   NewNetworkCollector(),
-		temp:      NewTemperatureCollector(),
-		throttled: NewThrottledCollector(),
+		temp:      NewTemperatureCollector(vcg),
+		throttled: NewThrottledCollector(vcg),
 		sysInfo:   NewSysInfoCollector(),
 		updates:   NewUpdatesCollector(cfg.UpdatesStaleThreshold),
 		uptime:    NewUptimeCollector(),

internal/collector/temperature.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -77,35 +76,33 @@ func readThermalZoneMilliC(zonePath string) (float64, error) {
 // TemperatureCollector reads CPU temperature from sysfs, with an optional
 // vcgencmd-sourced GPU/SoC reading on Raspberry Pi OS.
 //
-// The thermal zone and vcgencmd path are resolved lazily and re-resolved
-// (throttled) when they are still missing, so a sensor or driver that
-// appears after the process started — e.g. a thermal module loaded late in
-// boot, or a zone path that changes across a kernel/driver update — is
-// picked up without restarting the collector.
+// The thermal zone is resolved lazily and re-resolved (throttled) when it
+// is still missing, so a sensor or driver that appears after the process
+// started — e.g. a thermal module loaded late in boot, or a zone path that
+// changes across a kernel/driver update — is picked up without restarting
+// the collector. vcgencmd detection is handled by the shared vcg runner,
+// which TemperatureCollector and ThrottledCollector both use.
 type TemperatureCollector struct {
 	zoneGlob string // sysfs glob for thermal zones (overridable in tests)
 
-	mu                 sync.Mutex
-	now                func() time.Time
-	zonePath           string
-	zoneType           string
-	lastZoneDetect     time.Time
-	vcgencmdPath       string // empty if vcgencmd is not available
-	vcgencmdDetected   bool   // whether a vcgencmd lookup has ever succeeded
-	lastVcgencmdDetect time.Time
+	mu             sync.Mutex
+	now            func() time.Time
+	zonePath       string
+	zoneType       string
+	lastZoneDetect time.Time
+	vcg            *vcgencmdRunner // nil disables the GPU/SoC reading
 }
 
-// NewTemperatureCollector auto-detects the CPU thermal zone and checks
-// whether vcgencmd is available. Detection failures are not fatal: the
-// collector still works, it just reports errors from Collect() until a
-// thermal zone appears (e.g. useful for local development off-Pi). If the
-// zone (or vcgencmd) is missing at construction, Collect re-attempts
-// detection at most once every detectRetryInterval, so a sensor that shows
-// up later is used automatically.
-func NewTemperatureCollector() *TemperatureCollector {
-	c := &TemperatureCollector{zoneGlob: thermalZoneGlob, now: time.Now}
+// NewTemperatureCollector auto-detects the CPU thermal zone. Detection
+// failure is not fatal: the collector still works, it just reports errors
+// from Collect() until a thermal zone appears (e.g. useful for local
+// development off-Pi). If the zone is missing at construction, Collect
+// re-attempts detection at most once every detectRetryInterval, so a sensor
+// that shows up later is used automatically. vcg is the vcgencmd runner
+// shared with ThrottledCollector; pass nil to disable the GPU/SoC reading.
+func NewTemperatureCollector(vcg *vcgencmdRunner) *TemperatureCollector {
+	c := &TemperatureCollector{zoneGlob: thermalZoneGlob, now: time.Now, vcg: vcg}
 	c.redetectZoneLocked()
-	c.redetectVcgencmdLocked()
 	return c
 }
 
@@ -127,23 +124,6 @@ func (c *TemperatureCollector) redetectZoneLocked() {
 	}
 }
 
-// redetectVcgencmdLocked retries exec.LookPath("vcgencmd") if it has never
-// been found, throttled the same way as zone re-detection.
-func (c *TemperatureCollector) redetectVcgencmdLocked() {
-	if c.vcgencmdDetected {
-		return
-	}
-	now := c.now()
-	if !c.lastVcgencmdDetect.IsZero() && now.Sub(c.lastVcgencmdDetect) < detectRetryInterval {
-		return
-	}
-	c.lastVcgencmdDetect = now
-	if path, err := exec.LookPath("vcgencmd"); err == nil {
-		c.vcgencmdPath = path
-		c.vcgencmdDetected = true
-	}
-}
-
 // Collect returns the current CPU temperature and, if vcgencmd is
 // available, the GPU/SoC temperature as a secondary reading.
 func (c *TemperatureCollector) Collect(ctx context.Context) (Temperature, *GPUTemperature, error) {
@@ -176,31 +156,26 @@ func (c *TemperatureCollector) Collect(ctx context.Context) (Temperature, *GPUTe
 	}
 	temp := Temperature{Zone: c.zoneType, Celsius: celsius}
 
-	c.redetectVcgencmdLocked()
-	if c.vcgencmdPath == "" {
-		return temp, nil, nil
-	}
-
 	gpuTemp, err := c.readVcgencmdTemp(ctx)
 	if err != nil {
-		// vcgencmd is an optional extra data point; its failure should not
-		// fail the whole collection.
+		// vcgencmd is an optional extra data point; its unavailability or
+		// failure should not fail the whole collection.
 		return temp, nil, nil
 	}
 	return temp, &gpuTemp, nil
 }
 
-// readVcgencmdTemp runs `vcgencmd measure_temp` and parses output of the
-// form "temp=42.8'C".
+// readVcgencmdTemp runs `vcgencmd measure_temp` (via the shared vcg runner)
+// and parses output of the form "temp=42.8'C".
 func (c *TemperatureCollector) readVcgencmdTemp(ctx context.Context) (GPUTemperature, error) {
-	ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
-	defer cancel()
-
-	out, err := exec.CommandContext(ctx, c.vcgencmdPath, "measure_temp").Output()
+	if c.vcg == nil {
+		return GPUTemperature{}, errVcgencmdUnavailable
+	}
+	out, err := c.vcg.run(ctx, "measure_temp")
 	if err != nil {
-		return GPUTemperature{}, fmt.Errorf("run vcgencmd: %w", err)
+		return GPUTemperature{}, err
 	}
-	return parseVcgencmdTemp(string(out))
+	return parseVcgencmdTemp(out)
 }
 
 func parseVcgencmdTemp(output string) (GPUTemperature, error) {

internal/collector/temperature_test.go

@@ -102,6 +102,52 @@ func TestTemperatureCollector_Collect(t *testing.T) {
 	}
 }
 
+func TestTemperatureCollector_Collect_WithGPUTemp(t *testing.T) {
+	root := t.TempDir()
+	writeThermalZone(t, root, "thermal_zone0", "cpu-thermal", "50000")
+	scriptDir := t.TempDir()
+	path := writeFakeVcgencmd(t, scriptDir, "fake-vcgencmd", `echo "temp=42.8'C"`)
+
+	c := &TemperatureCollector{
+		zonePath: filepath.Join(root, "thermal_zone0"),
+		zoneType: "cpu-thermal",
+		vcg:      &vcgencmdRunner{detected: true, path: path},
+	}
+	temp, gpuTemp, err := c.Collect(context.Background())
+	if err != nil {
+		t.Fatalf("Collect: %v", err)
+	}
+	if diffFloat(temp.Celsius, 50.0) > 0.001 {
+		t.Fatalf("Celsius = %v, want 50.0", temp.Celsius)
+	}
+	if gpuTemp == nil || diffFloat(gpuTemp.Celsius, 42.8) > 0.001 {
+		t.Fatalf("gpuTemp = %+v, want Celsius=42.8", gpuTemp)
+	}
+}
+
+func TestTemperatureCollector_Collect_VcgencmdExecFails(t *testing.T) {
+	root := t.TempDir()
+	writeThermalZone(t, root, "thermal_zone0", "cpu-thermal", "50000")
+	scriptDir := t.TempDir()
+	path := writeFakeVcgencmd(t, scriptDir, "fake-vcgencmd", "exit 1")
+
+	c := &TemperatureCollector{
+		zonePath: filepath.Join(root, "thermal_zone0"),
+		zoneType: "cpu-thermal",
+		vcg:      &vcgencmdRunner{detected: true, path: path},
+	}
+	temp, gpuTemp, err := c.Collect(context.Background())
+	if err != nil {
+		t.Fatalf("Collect: %v", err)
+	}
+	if diffFloat(temp.Celsius, 50.0) > 0.001 {
+		t.Fatalf("Celsius = %v, want 50.0", temp.Celsius)
+	}
+	if gpuTemp != nil {
+		t.Fatalf("expected no GPU temp when vcgencmd exec fails, got %+v", gpuTemp)
+	}
+}
+
 func TestTemperatureCollector_Collect_NoZoneDetected(t *testing.T) {
 	c := &TemperatureCollector{}
 	if _, _, err := c.Collect(context.Background()); err == nil {
@@ -115,9 +161,9 @@ func TestTemperatureCollector_Collect_RedetectsZone(t *testing.T) {
 
 	now := time.Unix(1_700_000_000, 0)
 	c := &TemperatureCollector{
-		zoneGlob:         glob,
-		now:              func() time.Time { return now },
-		vcgencmdDetected: true, // skip vcgencmd lookup in this test
+		zoneGlob: glob,
+		now:      func() time.Time { return now },
+		// vcg left nil: skips vcgencmd entirely for this test.
 	}
 
 	// No zone exists yet: Collect must fail.
@@ -151,9 +197,9 @@ func TestTemperatureCollector_Collect_RedetectsAfterZoneVanishes(t *testing.T) {
 
 	now := time.Unix(1_700_000_000, 0)
 	c := &TemperatureCollector{
-		zoneGlob:         glob,
-		now:              func() time.Time { return now },
-		vcgencmdDetected: true, // skip vcgencmd lookup in this test
+		zoneGlob: glob,
+		now:      func() time.Time { return now },
+		// vcg left nil: skips vcgencmd entirely for this test.
 	}
 
 	// A zone exists at first and is cached by Collect.

internal/collector/testhelpers_test.go

@@ -24,3 +24,17 @@ func overwriteTempFile(t *testing.T, path, content string) {
 		t.Fatalf("overwrite temp file %s: %v", path, err)
 	}
 }
+
+// writeFakeVcgencmd writes an executable shell script standing in for the
+// real vcgencmd binary (mirroring the "true"/"false" stub-command pattern
+// used in updates_test.go) and returns its path. script is the script body,
+// e.g. `echo "temp=42.8'C"` or `exit 1`, and runs regardless of the
+// subcommand argument it's invoked with.
+func writeFakeVcgencmd(t *testing.T, dir, name, script string) string {
+	t.Helper()
+	path := filepath.Join(dir, name)
+	if err := os.WriteFile(path, []byte("#!/bin/sh\n"+script+"\n"), 0o755); err != nil {
+		t.Fatalf("write fake vcgencmd %s: %v", path, err)
+	}
+	return path
+}

internal/collector/throttled.go

@@ -2,12 +2,10 @@ package collector
 
 import (
 	"context"
+	"errors"
 	"fmt"
-	"os/exec"
 	"strconv"
 	"strings"
-	"sync"
-	"time"
 )
 
 // Bit positions in the bitmask reported by `vcgencmd get_throttled`. The
@@ -31,70 +29,39 @@ const (
 // systems without vcgencmd (e.g. development machines) it degrades to no
 // reading rather than failing.
 //
-// The vcgencmd path is resolved lazily and re-resolved (throttled to at most
-// once per detectRetryInterval) while it is still missing, mirroring
-// TemperatureCollector, so a firmware tool that appears after startup is
-// picked up without restarting the collector.
+// vcgencmd detection/execution is delegated to a vcgencmdRunner shared with
+// TemperatureCollector, so the lazy detection/re-detection logic and the
+// exec.LookPath("vcgencmd") call both live and run in one place rather than
+// being duplicated per collector.
 type ThrottledCollector struct {
-	mu                 sync.Mutex
-	now                func() time.Time
-	vcgencmdPath       string // empty if vcgencmd is not available
-	vcgencmdDetected   bool   // whether a vcgencmd lookup has ever succeeded
-	lastVcgencmdDetect time.Time
+	vcg *vcgencmdRunner // nil disables collection entirely
 }
 
-// NewThrottledCollector checks whether vcgencmd is available. A missing
-// vcgencmd is not fatal: Collect simply returns no reading until vcgencmd
-// appears (re-detection is retried at most once per detectRetryInterval).
-func NewThrottledCollector() *ThrottledCollector {
-	c := &ThrottledCollector{now: time.Now}
-	c.redetectVcgencmdLocked()
-	return c
+// NewThrottledCollector wraps vcg, the vcgencmd runner shared with
+// TemperatureCollector. A missing vcgencmd is not fatal: Collect simply
+// returns no reading until vcgencmd appears (re-detection is retried by vcg
+// at most once per detectRetryInterval). Pass nil to disable collection.
+func NewThrottledCollector(vcg *vcgencmdRunner) *ThrottledCollector {
+	return &ThrottledCollector{vcg: vcg}
 }
 
-// redetectVcgencmdLocked retries exec.LookPath("vcgencmd") if it has never
-// been found, throttled to at most once per detectRetryInterval. Caller must
-// hold c.mu (the constructor is single-threaded, so it also qualifies).
-func (c *ThrottledCollector) redetectVcgencmdLocked() {
-	if c.vcgencmdDetected {
-		return
-	}
-	now := c.now()
-	if !c.lastVcgencmdDetect.IsZero() && now.Sub(c.lastVcgencmdDetect) < detectRetryInterval {
-		return
-	}
-	c.lastVcgencmdDetect = now
-	if path, err := exec.LookPath("vcgencmd"); err == nil {
-		c.vcgencmdPath = path
-		c.vcgencmdDetected = true
-	}
-}
-
-// Collect runs `vcgencmd get_throttled` and decodes the bitmask. It returns
-// (nil, nil) when vcgencmd is not available, so the throttled object is
-// simply omitted from the snapshot off-Pi.
+// Collect runs `vcgencmd get_throttled` (via the shared vcg runner) and
+// decodes the bitmask. It returns (nil, nil) when vcgencmd is not
+// available, so the throttled object is simply omitted from the snapshot
+// off-Pi.
 func (c *ThrottledCollector) Collect(ctx context.Context) (*Throttled, error) {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-
-	// Collectors built as struct literals in tests may not set the clock.
-	if c.now == nil {
-		c.now = time.Now
-	}
-
-	c.redetectVcgencmdLocked()
-	if c.vcgencmdPath == "" {
+	if c.vcg == nil {
 		return nil, nil
 	}
 
-	ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
-	defer cancel()
-
-	out, err := exec.CommandContext(ctx, c.vcgencmdPath, "get_throttled").Output()
+	out, err := c.vcg.run(ctx, "get_throttled")
 	if err != nil {
-		return nil, fmt.Errorf("run vcgencmd get_throttled: %w", err)
+		if errors.Is(err, errVcgencmdUnavailable) {
+			return nil, nil
+		}
+		return nil, err
 	}
-	t, err := parseThrottled(string(out))
+	t, err := parseThrottled(out)
 	if err != nil {
 		return nil, err
 	}