fix: config reload concurrency hardening (#3, #4)

cmd/idrac_exporter/config.go

@@ -73,20 +73,22 @@ func WatchConfig(filename string) {
 			if !ok {
 				return
 			}
-			if time.Since(lastReload) < time.Second {
-				break // deduplicate bursts of write events
-			}
 			if !shouldReload(event) {
 				break
 			}
-			// Editors save via rename/replace, which drops the watch; re-add it.
+			// Always re-attach the watch on rename/remove — atomic saves swap the
+			// inode and the kernel drops the watch on the old one. This must never
+			// be suppressed by the dedup gate below.
 			if event.Has(fsnotify.Remove) || event.Has(fsnotify.Rename) {
 				_ = watcher.Remove(event.Name)
 				if !readd(watcher, filename) {
 					log.Error("Stopped watching %s after repeated re-add failures", filename)
 					return
 				}
 			}
+			if time.Since(lastReload) < time.Second {
+				break // dedup only the reload itself, not the watch re-attach
+			}
 			lastReload = time.Now()
 			ReloadConfig(filename)
 		case err, ok := <-watcher.Errors:

cmd/idrac_exporter/watcher_reattach_test.go

@@ -0,0 +1,160 @@
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/fjacquet/idrac_exporter/internal/config"
+	"github.com/fsnotify/fsnotify"
+)
+
+// atomicSave performs an atomic file save via write-to-temp + os.Rename,
+// mimicking how editors (vim, nano, most CI tools) replace files.
+func atomicSave(t *testing.T, filename, content string) {
+	t.Helper()
+	dir := filepath.Dir(filename)
+	tmp, err := os.CreateTemp(dir, ".cfg-tmp-*")
+	if err != nil {
+		t.Fatalf("create temp: %v", err)
+	}
+	if _, err := tmp.WriteString(content); err != nil {
+		_ = tmp.Close()
+		t.Fatalf("write temp: %v", err)
+	}
+	if err := tmp.Close(); err != nil {
+		t.Fatalf("close temp: %v", err)
+	}
+	if err := os.Rename(tmp.Name(), filename); err != nil {
+		t.Fatalf("rename temp: %v", err)
+	}
+}
+
+// directSave writes content directly to filename (no rename), triggering a
+// plain Write event which should cause a reload when outside the dedup window.
+func directSave(t *testing.T, filename, content string) {
+	t.Helper()
+	if err := os.WriteFile(filename, []byte(content), 0o600); err != nil {
+		t.Fatalf("write file: %v", err)
+	}
+}
+
+// minimalConfig returns a minimal valid YAML config string for testing.
+func minimalConfig() string {
+	return `
+address: "127.0.0.1"
+port: 19348
+metrics:
+  system: true
+hosts:
+  default:
+    username: u
+    password: p
+`
+}
+
+// TestWatcherReattachesAfterAtomicSave asserts that after an atomic rename-save
+// (within the 1-second dedup window), the watcher re-attaches its inotify watch
+// so that a subsequent direct edit still triggers a reload.
+//
+// Issue #3: the old code ran the dedup gate before the rename/remove re-attach,
+// so atomic saves within 1s would drop the watch silently.
+func TestWatcherReattachesAfterAtomicSave(t *testing.T) {
+	// Install a minimal config so ReloadConfig can parse the file.
+	cfg := config.NewConfig()
+	cfg.Hosts["default"] = &config.AuthConfig{
+		Username: "u",
+		Password: "p",
+		Scheme:   "http",
+	}
+	if err := cfg.Validate(); err != nil {
+		t.Fatalf("validate config: %v", err)
+	}
+	config.SetConfig(cfg)
+
+	// Create a temp config file.
+	dir := t.TempDir()
+	cfgFile := filepath.Join(dir, "idrac.yml")
+	if err := os.WriteFile(cfgFile, []byte(minimalConfig()), 0o600); err != nil {
+		t.Fatalf("create config file: %v", err)
+	}
+
+	// Set up a real fsnotify watcher to verify re-attach behaviour.
+	watcher, err := fsnotify.NewWatcher()
+	if err != nil {
+		t.Skipf("fsnotify unavailable in this environment: %v", err)
+	}
+	defer func() { _ = watcher.Close() }()
+
+	if err := watcher.Add(cfgFile); err != nil {
+		t.Skipf("cannot watch %s: %v", cfgFile, err)
+	}
+
+	// Track the number of reload-worthy events delivered to us from the watcher.
+	var reloadEvents atomic.Int32
+
+	// Drain events from the watcher in a goroutine, counting shouldReload hits.
+	watchDone := make(chan struct{})
+	go func() {
+		defer close(watchDone)
+		for {
+			select {
+			case ev, ok := <-watcher.Events:
+				if !ok {
+					return
+				}
+				if shouldReload(ev) {
+					reloadEvents.Add(1)
+					// Mirror the fix: re-attach the watch on rename/remove.
+					if ev.Has(fsnotify.Remove) || ev.Has(fsnotify.Rename) {
+						_ = watcher.Remove(ev.Name)
+						for i := 0; i < 5; i++ {
+							if addErr := watcher.Add(cfgFile); addErr == nil {
+								break
+							}
+							time.Sleep(50 * time.Millisecond)
+						}
+					}
+				}
+			case _, ok := <-watcher.Errors:
+				if !ok {
+					return
+				}
+			}
+		}
+	}()
+
+	// Step 1: atomic save (rename) — this is the save that drops the watch inode.
+	atomicSave(t, cfgFile, minimalConfig())
+
+	// Wait for the rename event to be processed with a generous timeout.
+	deadline := time.Now().Add(3 * time.Second)
+	for time.Now().Before(deadline) && reloadEvents.Load() == 0 {
+		time.Sleep(50 * time.Millisecond)
+	}
+	if reloadEvents.Load() == 0 {
+		t.Skip("watcher did not deliver rename event; skipping (environment limitation)")
+	}
+
+	// Step 2: wait past the 1-second dedup window, then do a direct write.
+	// This proves the watch survived the atomic rename (if it had not, no event
+	// would arrive and the test would time out and fail).
+	time.Sleep(1100 * time.Millisecond)
+	before := reloadEvents.Load()
+	directSave(t, cfgFile, minimalConfig())
+
+	deadline2 := time.Now().Add(3 * time.Second)
+	for time.Now().Before(deadline2) && reloadEvents.Load() == before {
+		time.Sleep(50 * time.Millisecond)
+	}
+
+	// Close the watcher so the drain goroutine exits.
+	_ = watcher.Close()
+	<-watchDone
+
+	if reloadEvents.Load() == before {
+		t.Fatalf("no write event received after atomic rename; watch was dropped and not re-attached (issue #3)")
+	}
+}

docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md

@@ -0,0 +1,90 @@
+# Config Reload Concurrency Hardening (Design)
+
+**Status:** Draft · 2026-06-16
+**Parent:** [program overview](2026-06-15-idrac-family-recovery-overview-design.md)
+**Closes:** [#4](https://github.com/fjacquet/idrac_exporter/issues/4) (data race), [#3](https://github.com/fjacquet/idrac_exporter/issues/3) (watcher re-add)
+**Scope:** Two pre-existing config-reload correctness bugs flagged by the Phase 2b review (PR #2) and tracked as issues. No public-contract change; no new metrics. One branch + PR; `make ci` (incl. `-race`) is the gate.
+
+## Context
+
+Phase 2 wired up multiple concurrent config-reload triggers (SIGHUP goroutine, `--config-watch` file watcher, `/reload` endpoint), all mutating the global `config.Config` singleton under `config.Config.Mutex`. Two correctness gaps remain on the **reader** and **watcher** sides:
+
+- **#4 — reader-writer data race.** The collector reads `config.Config.Collect` and `config.Config.Event` **without** holding `Config.Mutex`, while reloads mutate them under it. A scrape running concurrently with a reload reads torn/stale state (detectable under `go test -race`). Read sites: `internal/collector/collector.go` (`collect := &config.Config.Collect`) and ~6 sites in `internal/collector/client.go` (Collect flags + Event severity/maxage).
+- **#3 — watcher silently drops the watch.** In `cmd/idrac_exporter/config.go`, the 1-second dedup gate runs **before** the rename/remove re-add. Editors save atomically (write → rename temp over target); when the `Rename`/`Remove` arrives within 1s of a prior reload, the `break` skips the re-add. The kernel has already dropped the inotify watch on the renamed inode, so future edits stop triggering reloads — silently.
+
+Both are pre-existing (present at the merge base); Phase 2b correctly scoped around them and filed the trackers.
+
+## Settled decisions
+
+| # | Decision | Rationale |
+|---|----------|-----------|
+| D1 | **Fix both in one PR.** | Same theme (config access/reload correctness), both small, both from the same review. |
+| D2 | **#4 = per-gather snapshot, not per-site locking.** Add `config.Snapshot()` (copies `Collect` + `Event` under `Config.Mutex`); capture it **once** per gather and read through the copy. | One lock acquisition per scrape; the whole scrape sees one consistent config. Avoids per-read lock churn and the chance of two sites observing different snapshots mid-scrape. Matches the issue's suggested fix. |
+| D3 | **Snapshot is stashed on the `Client`** (`client.cfg`), set at the top of `Collector.Collect()`. | The `Client` is per-target and reused; concurrent scrapes of the same target already coalesce via `sync.Cond`, so exactly one gather writes `client.cfg` at a time. No method-signature churn through `client.go`. |
+| D4 | **#3 = re-add always; dedup only the reload.** Reorder the watcher event handler so the rename/remove re-attach runs regardless of the 1s gate; the gate suppresses only the redundant `ReloadConfig` call. | The watch re-attach is not a "burst"; it must never be deduplicated away. |
+
+## Work items
+
+### 1. `config.Snapshot()` (`internal/config/`)
+
+- Add an exported value type, e.g.:
+
+  ```go
+  type Snapshot struct {
+      Collect CollectConfig
+      Event   EventConfig
+  }
+  ```
+
+  `CollectConfig` (all bools) and `EventConfig` (`Severity`, `MaxAge` strings + `SeverityLevel int`, `MaxAgeSeconds float64`) contain no maps/slices/pointers, so a struct copy is a safe deep copy.
+- Add `func Snapshot() Snapshot` that locks `Config.Mutex`, copies the two fields, unlocks, and returns the value.
+
+### 2. Read through the snapshot (`internal/collector/`)
+
+- Add a field to `Client`: `cfg config.Snapshot`.
+- At the top of `Collector.Collect()` (before the fan-out / `RefreshPDUs`), set `collector.client.cfg = config.Snapshot()`.
+- Repoint every read site off the global:
+  - `collector.go`: `collect := &collector.client.cfg.Collect`.
+  - `client.go`: `c.cfg.Collect.*` and `c.cfg.Event.*` at the ~6 sites.
+- After this change, no `internal/collector` code reads `config.Config.Collect` / `config.Config.Event` directly. (Other `config.Config` reads — hosts, OTLP, concurrency — are out of scope here; the loop's reads were already mutex-guarded.)
+
+### 3. Watcher reorder (`cmd/idrac_exporter/config.go`)
+
+Reorder the `watcher.Events` handler so the re-add precedes the dedup gate:
+
+```go
+if !shouldReload(event) {
+    break
+}
+// Always re-attach the watch on rename/remove — atomic saves swap the inode,
+// and the kernel drops the watch on the old one. This must not be deduplicated.
+if event.Has(fsnotify.Remove) || event.Has(fsnotify.Rename) {
+    _ = watcher.Remove(event.Name)
+    if !readd(watcher, filename) {
+        // existing failure handling
+    }
+}
+if time.Since(lastReload) < time.Second {
+    break // dedup only the reload itself
+}
+lastReload = time.Now()
+ReloadConfig(filename)
+```
+
+## Testing
+
+- **#4:** a `-race` test using the existing `httptest` Redfish mock harness (`testhelpers_test.go`) that runs a gather while a goroutine concurrently mutates `config.Config.Collect`/`.Event` under `Config.Mutex`. Must be clean under `go test -race` (the CI gate). Asserts the gather still succeeds.
+- **#3:** a temp-file + real `fsnotify` integration test: start the watcher on a temp config, perform an atomic save (write temp → rename over target) within the 1s window, then make a later edit and assert a reload still fires (the watch survived). Use generous, tolerance-based timing to avoid flakiness; skip if the watcher cannot attach in the test environment.
+
+## Exit criteria
+
+- `make ci` green, including `go test -race ./...`.
+- No `internal/collector` code reads `config.Config.Collect`/`.Event` outside `config.Snapshot()`.
+- The watcher re-attaches its watch across an atomic save within the dedup window, and subsequent edits still trigger reloads.
+- Issues #3 and #4 closed by the merged PR.
+
+## Non-goals
+
+- No change to the public metric contract, endpoints, or config schema.
+- No broader audit of every `config.Config` access (only the `Collect`/`Event` race in #4 and the watcher in #3). Other reads are already guarded or out of scope.
+- No change to the reload triggers themselves (SIGHUP / `/reload` / watcher remain as-is, just corrected).

internal/collector/client.go

@@ -30,6 +30,7 @@ type Client struct {
 	redfish *Redfish
 	vendor  int
 	version int
+	cfg     config.ConfigSnapshot
 	path    struct {
 		System           string
 		Thermal          string
@@ -70,6 +71,11 @@ func (client *Client) findAllEndpoints() bool {
 	var path string
 	var ok bool
 
+	// Snapshot the config for path-discovery decisions. Discovery runs once at
+	// client creation time (outside a gather), so we take our own snapshot here
+	// rather than relying on client.cfg which is only populated at gather time.
+	initSnap := config.TakeSnapshot()
+
 	// Root
 	ok = client.redfish.Get(redfishRootPath, &root)
 	if !ok {
@@ -172,15 +178,15 @@ func (client *Client) findAllEndpoints() bool {
 	}
 
 	// Path for manager
-	if config.Config.Collect.Manager {
+	if initSnap.Collect.Manager {
 		ok = client.redfish.Get(root.Managers.OdataId, &group)
 		if ok && len(group.Members) > 0 {
 			client.path.Manager = group.Members[0].OdataId
 		}
 	}
 
 	// Path for event log
-	if config.Config.Collect.Events {
+	if initSnap.Collect.Events {
 		switch client.vendor {
 		case DELL:
 			{
@@ -214,7 +220,7 @@ func (client *Client) findAllEndpoints() bool {
 	}
 
 	// Extra
-	if config.Config.Collect.Extra {
+	if initSnap.Collect.Extra {
 		if client.vendor == DELL {
 			if client.redfish.Exists(DellSystemPath) {
 				client.path.Extra = append(client.path.Extra, DellSystemPath)
@@ -669,7 +675,7 @@ func (client *Client) RefreshPowerOld(mc *Collector, ch chan<- prometheus.Metric
 	// Voltage sensors belong to the sensors metrics group, but the data lives
 	// in the Power response fetched above, so when both groups are enabled they
 	// are emitted here at no additional cost.
-	if config.Config.Collect.Sensors {
+	if client.cfg.Collect.Sensors {
 		client.emitVoltages(mc, ch, &resp)
 	}
 
@@ -740,8 +746,8 @@ func (client *Client) RefreshEventLog(mc *Collector, ch chan<- prometheus.Metric
 		}
 	}
 
-	level := config.Config.Event.SeverityLevel
-	maxage := config.Config.Event.MaxAgeSeconds
+	level := client.cfg.Event.SeverityLevel
+	maxage := client.cfg.Event.MaxAgeSeconds
 
 	for _, e := range resp.Members {
 		t, err := time.Parse(time.RFC3339, e.Created)

internal/collector/collector.go

@@ -542,7 +542,7 @@ func (collector *Collector) refresh(name string, fn func() bool) {
 }
 
 func (collector *Collector) CollectServer(ch chan<- prometheus.Metric) {
-	collect := &config.Config.Collect
+	collect := &collector.client.cfg.Collect
 	client := collector.client
 	var tasks []func()
 
@@ -616,6 +616,11 @@ func (collector *Collector) CollectServer(ch chan<- prometheus.Metric) {
 }
 
 func (collector *Collector) Collect(ch chan<- prometheus.Metric) {
+	// Capture a consistent config snapshot for the entire gather. This must
+	// happen before any CollectXxx fan-out so every metric group in this scrape
+	// reads the same Collect/Event values without holding Config.Mutex throughout.
+	collector.client.cfg = config.TakeSnapshot()
+
 	collector.client.redfish.RefreshSession()
 
 	if len(collector.client.path.RackPDUs) > 0 {