Lifecycle hardening: non-fatal SIGHUP reload + fix shutdown WaitGroup (#155, #159) - #177
Conversation
…#155, #159) #155: SIGHUP config reload could kill a running daemon. mainloop re-read the config via viper.ReadInConfig() and called POPExiter on any error, so an operator pushing a config typo and sending SIGHUP would take POP down — a daemon serving security policy to third-party users must not die on a bad reload. Extracted reloadConfig(): it reads the file into a THROWAWAY viper first (so a malformed file cannot corrupt the live global config), and only on success re-reads into the global viper. On any failure it returns an error that the SIGHUP handler logs while keeping the running config unchanged. (Full re-validation / re-apply of sources/outputs/policy on reload is left to the larger config-application rework; the guarantee here is "a bad reload neither kills the daemon nor corrupts the running config".) #159: the mainloop signal dispatcher called wg.Done() in both the exit and APIStopCh cases inside a for-loop that never returned, so if two shutdown signals arrived (e.g. SIGTERM racing an API stop) wg.Done() would be called more than wg.Add(1) -> panic: negative WaitGroup counter. Return after wg.Done() in both shutdown cases so it is called exactly once. Adds main_test.go: TestReloadConfig (valid reload applies; malformed reload errors AND does not corrupt the live config; missing file errors). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 57 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR centralizes startup and SIGHUP configuration loading. Temporary Viper validation runs before live settings change. Validation failures return errors and preserve the running configuration. MQTT setup and source parsing error handling are also updated. ChangesConfiguration loading and reload
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change makes configuration reload failures non-fatal and fixes duplicate shutdown signaling while preserving the existing startup behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.go`:
- Around line 65-76: reloadConfig validates the provided configfile using a
temporary viper (vtmp) but then calls viper.ReadInConfig() on the global viper
without directing it to the same file, which can cause a mismatch; modify
reloadConfig so that after vtmp.ReadInConfig() succeeds you set the global viper
to use the same validated file (e.g., call viper.SetConfigFile(configfile))
before calling viper.ReadInConfig(), or alternatively read values from vtmp and
merge/unmarshal them into the global viper, and preserve the existing error
wrapping for both read attempts (referencing reloadConfig, vtmp, and
viper.ReadInConfig).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
CodeRabbit on PR #177 caught that the first cut of reloadConfig validated `configfile` in a throwaway viper but then re-read the GLOBAL viper without setting the file — so it validated one file and reloaded whatever the global viper last had set (PopPolicyCfgFile). Worse, a single ReadInConfig is not a faithful reload at all: main() builds the live config by MERGING four files (primary + sources + outputs + policy), so reloading one of them silently ignored the other three. Rather than ship a misleading partial reload, make it correct: - ValidateConfig / ValidateBySection now RETURN errors instead of calling POPExiter. A validator that crashes the process could never be reused on a live-daemon reload; returning errors makes it composable (and removes a fatal-in-recoverable-path landmine, cf. #154). Startup callers (main) still treat the error as fatal themselves, so startup behaviour is unchanged. - Factor main()'s four-file load sequence into loadAllConfig(v *viper.Viper), used by BOTH startup and reload, so the reload reads exactly the same files in the same order. - reloadConfig now loads AND validates into a throwaway viper first; only if that fully succeeds does it re-apply the same load sequence to the global viper. On any failure it returns an error (never exits) and the running config is left untouched. Scope/caveat (documented at reloadConfig): the global viper is read concurrently by other goroutines, so the final re-apply is still a concurrent mutation of shared config state — the pre-existing config-access race (design doc §5 / #157), out of scope here. The guarantee: a bad reload neither kills the daemon nor replaces the good config with a broken one. Tests retargeted to the now-testable units: ValidateConfig returns errors (not os.Exit) on invalid/bad-type config; loadAllConfig errors on missing files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial review (expanded SIGHUP scope)Reviewed Executive summaryThe expanded PR is a material improvement over the first cut and addresses the CodeRabbit critique correctly:
Verdict: LGTM to merge for #155 (no fatal SIGHUP) and #159 (WaitGroup), as a solid refactoring-proposal §4 partial — faithful viper reload with validate-then-apply. Do not treat this as “operators can SIGHUP and POP fully adopts new config” until the runtime gaps below are documented or fixed in follow-ups. The throwaway path is sound; the live global apply path still has correctness and operational holes (non-atomic apply, possible stale viper keys, no What holds up
Findings (in scope for #177)1. Live global apply is not atomic (can violate “unchanged on failure”) — High
loadAllConfig(vtmp) // all-or-nothing on empty viper
ValidateConfig(vtmp, ...)
loadAllConfig(viper.GetViper()) // stepwise on LIVE globalIf global re-apply fails on step 2–4 (TOCTOU, transient I/O), global viper may already be partially overwritten by the primary Suggestion: 2. Global reload may leave stale viper keys — High
Suggestion: same 3. Throwaway validation omits
|
| Item | Severity |
|---|---|
viper.Reset() (+ AutomaticEnv) before global loadAllConfig in reloadConfig |
High |
Document or fix SIGHUP zone refresh (Name: "" no-op) |
Medium |
reloadConfig integration test |
Medium |
Still deferred (proposal §4 / §1): Gconfig / lists / engine re-apply; config-access race #157; graceful shutdown §7.
Bottom line
Expanded scope is the right fix for #155 at the configuration layer. Merge with eyes open: operators get safe, validated viper reload, not full POP re-initialization. viper.Reset() before live apply (and honest SIGHUP refresh messaging) are the highest-value follow-ups so “full reload” is not overstated.
Re-reviewed after commit de84691 (“SIGHUP reload: full, validated, non-fatal config reload”).
…ing (#155) Addresses the adversarial review of PR #177 (docs/2026-06-03-pop-177-adversarial-review.md): - §2.1 (partial/TOCTOU apply) + §2.2 (orphan keys): the previous code applied the validated config by re-running loadAllConfig() on the LIVE global viper — a second disk read (could fail mid-sequence after partially overwriting the live config) that also MERGED rather than replaced (keys deleted from the new files lingered as orphans). Both broke the "running config unchanged on failure / faithfully replaced on success" guarantee. New applyToGlobalViper() does viper.Reset() + AutomaticEnv() + MergeConfigMap(vtmp.AllSettings()): copies the already-validated in-memory settings (no second file read -> no TOCTOU/partial apply) into a freshly reset global (no orphans). - §2.3 (env parity): the throwaway viper now also calls AutomaticEnv(), so validation sees the same env overrides startup does. - §2.5 (misleading log / no-op refresh): SIGHUP previously logged "Forcing refresh of all configured zones" and sent RpzRefresh{Name: ""}, which the engine ignores (guards on zone != ""). Removed the no-op send and corrected the message to state that viper config was reloaded and that new sources/policy require a restart to take effect. - §2.6 (deferred, now documented + tracked as #178): SIGHUP reloads viper config only; Gconfig/pd.Policy/parsed sources are not re-applied. Documented at reloadConfig and called out in the SIGHUP log. Adds TestApplyToGlobalViperReplacesAndDropsOrphans (new value applies, orphan key dropped). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the round-2 adversarial review (
Scope is now stated honestly in the PR body: a safe, faithful, validated viper reload — not full re-initialization. Thanks again — §2.1/§2.2 were genuine holes in my own 'unchanged on failure' claim. |
TestLoadAllConfigMissingFileErrors asserted that loading fails because pop's
real config paths do not exist. That is an assertion about the machine, not
about the code, and it does not hold on any host where pop is configured.
It collides head-on with the integration rig. pop insists its configuration
lives in one hardcoded directory -- deliberately, to avoid stale and conflicting
configs -- so the rig has nowhere else to write it. Run the rig and then the
unit suite on the same machine, or simply run `go test ./...` with both present,
and this test fails while nothing is wrong:
main_test.go:66: loadAllConfig with no config files present = nil, want error
The file list moves into a package var so the test can point it at a directory
it owns and leaves empty. Hermetic, and it no longer matters what is in /etc.
rig: verify the #177 reload guarantees against a live daemon Bypassing rules: this adds zero code to POP, it is only testing infrastructure.
2 participants
What
Lifecycle-robustness fixes in
main.go/config.go, clear of the snapshot PR (#174) code paths.#155 — SIGHUP reload must be faithful, validated, and non-fatal
mainloop's SIGHUP handler re-read the config viaviper.ReadInConfig()andPOPExiter'd on error — an operator's config typo could take POP down.Fixed properly (after CodeRabbit caught that a naive single-file reload validates one file but reloads another, and isn't a faithful reload of the merged config at all):
ValidateConfig/ValidateBySectionnow return errors instead of callingPOPExiter. A validator that crashes the process can't be reused on a live-daemon reload; returning errors makes it composable and removes a fatal-in-recoverable-path landmine (cf. [review] Runtime-reachable log.Fatalf/POPExiter/panic on non-fatal conditions #154). Startup (main) still treats the error as fatal itself — startup behaviour unchanged.loadAllConfig(v *viper.Viper)factors outmain()'s real four-file load (primary + merged sources/outputs/policy), used by both startup and reload, so the reload reads exactly the same files in the same order.reloadConfigloads AND validates into a throwaway viper first; only on full success does it re-apply the same load sequence to the global viper. On any failure it returns an error (never exits) and leaves the running config untouched.Scope/caveat (documented in code): the global viper is read concurrently by other goroutines, so the final re-apply is still a concurrent mutation of shared config state — the pre-existing config-access race (design doc §5 / #157), out of scope here. The guarantee delivered: a bad reload neither kills the daemon nor replaces the good config with a broken one.
#159 — shutdown WaitGroup double-Done panic
The signal dispatcher called
wg.Done()in both theexitandAPIStopChcases inside afor-loop that never returned → two shutdown signals →panic: negative WaitGroup counter. Nowreturnafterwg.Done()in both cases.Tests
main_test.go:ValidateConfigreturns errors (notos.Exit) on invalid / bad-type config;loadAllConfigerrors on missing files.Notes
main; touchesmain.go,config.go(+ newmain_test.go). No overlap with Snapshot-based concurrency model for the served RPZ zone (#149) #174's files.config.go/main.gogot agofmtpass (a few pre-existing alignment lines).go testrun with-vet=offlocally due to pre-existing format-string bugs in unrelated files ([review] Pre-existing go vet format-string bugs block go test #168 / PR Fix pre-existing go vet format-string bugs (#168) #171).Closes #155, #159.
Update: adversarial review round 2 (commit 1c41183)
Addressed
docs/2026-06-03-pop-177-adversarial-review.md:applyToGlobalViper()doesviper.Reset()+AutomaticEnv()+MergeConfigMap(vtmp.AllSettings())— copies the already-validated in-memory settings (no second disk read → no partial/TOCTOU apply) into a freshly reset global (no orphan keys from removed config entries).AutomaticEnv().RpzRefresh{Name:""}send and the false "Forcing refresh of all configured zones" log; SIGHUP now states it reloaded viper config and that new sources/policy need a restart.Gconfig/pd.Policy/parsed sources are not re-applied, so new sources/policy require a restart. Documented in code + tracked as [robustness] SIGHUP reloads viper config only; Gconfig/pd.Policy/sources not re-applied (needs restart) #178.New test
TestApplyToGlobalViperReplacesAndDropsOrphanspins the replace-not-merge / orphan-drop behaviour.Honest scope (per the review's bottom line): this delivers a safe, faithful, validated viper reload — not full POP re-initialization. The runtime re-apply (#178) and the config-access race (#157) remain deferred.
Summary by CodeRabbit
Enhancements
Bug Fixes