Skip to content

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

Merged
fjacquet merged 2 commits into
mainfrom
fix-config-reload-hardening
Jun 16, 2026
Merged

fix: config reload concurrency hardening (#3, #4)#14
fjacquet merged 2 commits into
mainfrom
fix-config-reload-hardening

Conversation

@fjacquet

Copy link
Copy Markdown
Owner

Fixes two pre-existing config-reload correctness bugs flagged by the Phase 2b review (PR #2), tracked as issues. No public-contract change.

Closes #3
Closes #4

#4 — collector data race

The collector read config.Config.Collect/.Event without Config.Mutex while SIGHUP / --config-watch / /reload mutate them under it — a reader-writer race under -race.

  • New config.TakeSnapshot() copies Collect+Event under Config.Mutex into a ConfigSnapshot value (both all-scalar structs → safe deep copy).
  • Captured once at the top of Collector.Collect() and stashed on Client.cfg, so the whole scrape sees one consistent config (concurrent same-target scrapes already coalesce). Path discovery (findAllEndpoints, runs at creation) uses its own local snapshot.
  • All internal/collector read sites repointed; grep -rn 'config\.Config\.\(Collect\|Event\)' internal/collector/ is now zero in production code.

#3 — watcher silently drops the watch

Atomic editor saves (write→rename) within the 1s dedup window hit the break before the rename/remove re-add, so the inotify watch was dropped and future edits stopped reloading. The handler now does the re-attach first, and the 1s gate suppresses only the ReloadConfig call.

Tests

  • internal/collector/config_race_test.go — gathers while a goroutine mutates Collect/Event under the mutex; clean under go test -race (would race on the old code).
  • cmd/idrac_exporter/watcher_reattach_test.go — real fsnotify: atomic-save within the window, then a later write still arrives (watch survived). Skips gracefully where the env can't attach a watch.
    • ⚠️ Note: this test exercises the re-attach technique via its own watcher goroutine rather than the unexported WatchConfig loop, so it won't catch a regression of WatchConfig itself. The production reorder is small and reviewed. A follow-up could extract the handler for direct testing.

Validation

  • make ci green — go vet, golangci-lint 0 issues, go test -race ./... (56 tests), govulncheck clean
  • semgrep clean on changed files

🤖 Generated with Claude Code

fjacquet and others added 2 commits June 16, 2026 06:45
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 <noreply@anthropic.com>
#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 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@fjacquet, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 5 minutes and 20 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 396ec5d5-0997-4975-839c-338d5f1f269f

📥 Commits

Reviewing files that changed from the base of the PR and between eb292c3 and 0f7166d.

📒 Files selected for processing (7)
  • cmd/idrac_exporter/config.go
  • cmd/idrac_exporter/watcher_reattach_test.go
  • docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md
  • internal/collector/client.go
  • internal/collector/collector.go
  • internal/collector/config_race_test.go
  • internal/config/config.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-config-reload-hardening

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@fjacquet fjacquet merged commit ab6705f into main Jun 16, 2026
4 checks passed
@fjacquet fjacquet deleted the fix-config-reload-hardening branch June 16, 2026 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant