From 2a1215595c2e1626a05f98f698c0fc58c3db94d1 Mon Sep 17 00:00:00 2001 From: Frederic Jacquet Date: Tue, 16 Jun 2026 06:45:12 +0200 Subject: [PATCH 1/2] docs: config reload concurrency hardening design (#3, #4) Per-gather config snapshot to kill the collector reader-writer race (#4), and reorder the watcher so the rename/remove re-add always runs ahead of the 1s dedup gate (#3). One PR; make ci (-race) is the gate. Co-Authored-By: Claude Opus 4.8 --- ...26-06-16-config-reload-hardening-design.md | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md diff --git a/docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md b/docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md new file mode 100644 index 0000000..62f051d --- /dev/null +++ b/docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md @@ -0,0 +1,87 @@ +# 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). From 0f7166d60424a4f80798c883c49021e7de5a4137 Mon Sep 17 00:00:00 2001 From: Frederic Jacquet Date: Tue, 16 Jun 2026 07:05:18 +0200 Subject: [PATCH 2/2] fix: config reload concurrency hardening (#3, #4) #4: collector now reads Collect/Event via a per-gather config snapshot (config.TakeSnapshot under Config.Mutex), captured once at the top of Collect() and stashed on Client.cfg; discovery uses its own local snapshot. No internal/collector code reads config.Config.Collect/.Event directly anymore. Kills the reader-writer data race under -race. #3: the config watcher now performs the rename/remove watch re-attach BEFORE the 1s dedup gate, so an atomic editor save within the window no longer silently drops the inotify watch. The gate now suppresses only the redundant ReloadConfig call. Tests: -race gather-vs-reload test (#4); real-fsnotify atomic-save re-attach test (#3). make ci green (vet, golangci-lint 0, test -race, govulncheck). Closes #3 Closes #4 Co-Authored-By: Claude Opus 4.8 --- cmd/idrac_exporter/config.go | 10 +- cmd/idrac_exporter/watcher_reattach_test.go | 160 ++++++++++++++++++ ...26-06-16-config-reload-hardening-design.md | 3 + internal/collector/client.go | 18 +- internal/collector/collector.go | 7 +- internal/collector/config_race_test.go | 66 ++++++++ internal/config/config.go | 22 +++ 7 files changed, 275 insertions(+), 11 deletions(-) create mode 100644 cmd/idrac_exporter/watcher_reattach_test.go create mode 100644 internal/collector/config_race_test.go diff --git a/cmd/idrac_exporter/config.go b/cmd/idrac_exporter/config.go index 8ca5551..0bf8dbb 100644 --- a/cmd/idrac_exporter/config.go +++ b/cmd/idrac_exporter/config.go @@ -73,13 +73,12 @@ 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) { @@ -87,6 +86,9 @@ func WatchConfig(filename string) { 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: diff --git a/cmd/idrac_exporter/watcher_reattach_test.go b/cmd/idrac_exporter/watcher_reattach_test.go new file mode 100644 index 0000000..da7f3fe --- /dev/null +++ b/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)") + } +} diff --git a/docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md b/docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md index 62f051d..c083e68 100644 --- a/docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md +++ b/docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md @@ -28,12 +28,14 @@ Both are pre-existing (present at the merge base); Phase 2b correctly scoped aro ### 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. @@ -49,6 +51,7 @@ Both are pre-existing (present at the merge base); Phase 2b correctly scoped aro ### 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 diff --git a/internal/collector/client.go b/internal/collector/client.go index 1a71f81..9849ddd 100644 --- a/internal/collector/client.go +++ b/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,7 +178,7 @@ 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 @@ -180,7 +186,7 @@ func (client *Client) findAllEndpoints() bool { } // 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) diff --git a/internal/collector/collector.go b/internal/collector/collector.go index 83e4c4f..a3f2a1d 100644 --- a/internal/collector/collector.go +++ b/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 { diff --git a/internal/collector/config_race_test.go b/internal/collector/config_race_test.go new file mode 100644 index 0000000..5dbb65f --- /dev/null +++ b/internal/collector/config_race_test.go @@ -0,0 +1,66 @@ +package collector + +import ( + "sync" + "testing" + + "github.com/fjacquet/idrac_exporter/internal/config" +) + +// TestCollectConfigSnapshotNoRace runs a gather while a goroutine concurrently +// mutates config.Config.Collect and config.Config.Event under Config.Mutex. +// It must be clean under go test -race (the CI gate) and the gather must +// succeed (fixing issue #4). +func TestCollectConfigSnapshotNoRace(t *testing.T) { + // Minimal config: enable System + Events so both Collect and Event fields + // are read during the gather, maximising the race-detector coverage. + testConfig(t, func(c *config.CollectConfig) { + c.System = true + c.Events = true + }) + + srv := mockRedfish(t, map[string]string{ + "/redfish/v1/Systems/1": "system.json", + }) + defer srv.Close() + + mc := NewCollector() + mc.client = testClient(srv) + mc.client.path.System = "/redfish/v1/Systems/1" + + var wg sync.WaitGroup + + // Mutator goroutine: repeatedly write Collect/Event under the mutex, + // simulating a concurrent config reload. + stop := make(chan struct{}) + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + config.Config.Mutex.Lock() + config.Config.Collect.System = true + config.Config.Collect.Events = true + config.Config.Event.SeverityLevel = 1 + config.Config.Event.MaxAgeSeconds = 604800 + config.Config.Mutex.Unlock() + } + } + }() + + // Run several gathers while the mutator is running. + for i := 0; i < 5; i++ { + _, err := mc.Gather() + if err != nil { + close(stop) + wg.Wait() + t.Fatalf("Gather() returned error: %v", err) + } + } + + close(stop) + wg.Wait() +} diff --git a/internal/config/config.go b/internal/config/config.go index 60278e3..63e8dbb 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,6 +15,28 @@ var Debug bool = false var Trace bool = false var Config *RootConfig = nil +// ConfigSnapshot holds a point-in-time copy of the fields that change on +// config reload and are read concurrently by the collector during a scrape. +// Both fields are all-scalar structs (bools, strings, ints, float64), so a +// plain struct copy is a safe deep copy — no pointers or maps involved. +type ConfigSnapshot struct { + Collect CollectConfig + Event EventConfig +} + +// TakeSnapshot locks Config.Mutex, copies the Collect and Event fields into a +// ConfigSnapshot value, unlocks, and returns the copy. The name avoids +// shadowing the Snapshot type or the method receiver. +func TakeSnapshot() ConfigSnapshot { + Config.Mutex.Lock() + snap := ConfigSnapshot{ + Collect: Config.Collect, + Event: Config.Event, + } + Config.Mutex.Unlock() + return snap +} + func (c *AuthConfig) Validate() error { if c == nil { return fmt.Errorf("empty section")