Refactor WatchConfig to panic inline instead of os.Exit() in a goroutine on init fail (#2095) by deefdragon · Pull Request #2096 · spf13/viper

deefdragon · 2025-11-21T20:37:27Z

Fixes #2095

I Chose to also add panics to the errors for the getConfigFile and watcher.Add parts as either of those failing was also returning, tho not exiting. This increases the consistency of this area of the code.

I am unsure if it would be better to just remove the panics and rely on the user not getting updates as expected. Unless there is some manner for listening to errors that occur in viper (an error channel etc) I think this is the best bet.

…ine on init fail Fixes spf13#2095 I Chose to also add panics to the errors for the getConfigFile and watcher.Add as either of those failing was also returning, tho not exiting. This increases the consistency of this area of teh code. I am unsure if it would be better to just remove the panics and rely on the user not getting updates as expected. Unless there is some manner for listening to errors that occur in viper (an error channel etc) I think this is the best bet.

CLAassistant · 2025-11-21T20:37:33Z

All committers have signed the CLA.

viper.go

ccoVeille · 2025-11-22T10:06:12Z

viper.go

-		eventsWG.Wait() // now, wait for event loop to end in this go-routine...
 	}()
-	initWG.Wait() // make sure that the go routine above fully ended before returning


I'm surprised by the changes about wait group here, and all the changes you did in the file to do not use waitgroup

I would have kept the change minimal to support removal the os.Exit, and that's it.

The other changes, which are good, I feel, are making the current PR uneasy to review.

I would have split this in 2 PRs one that can be easily be merged (the thing about panic), and a refactoring PR that could be reviewed and merged separately.

I'm not suggesting to change anything right now. I report here how I would have done it.
Other reviewers or you could have different opinions.

Also, I can be simply wrong and the changes here are somehow needed to address the os.Exit issue.

I really didn't want to refactor the wait groups & goroutines as I hate the resulting diff due to the tabbing. Unfortunately I honestly felt that it was necessary. 1: It was what allowed the panic calls to propagate up a reasonable stack (being recoverable by the calling user), and 2: it leaves the code in a much more simplified, and thus more readable/maintainable state.

As was with the wait groups, it was literally just synchronous code made excessively complicated to allow being lazy with the watcher.Close(), using the defer instead of just closing where it should be closed on error.

Thanks for confirming what I thought about the fact the code had no need for such asynchronous thing.

You are right about the stack in the panic, it makes sense.

So here, I would like to suggest you to split the first commit in two.

the first one to remove the complexity and keeping the os.Exit with a clear commit message about it's a refactoring made for simplifying over - complicated code

then a commit about the change about os.Exit.

(They could be inverted of course)

This way the PR diff will stay unchanged, but at least the history would be way clearer

…panic

deefdragon · 2025-11-22T10:30:53Z

@ccoVeille Do you think I should update the method comment (and corresponding function comment) to document that WatchConfig will panic on an error being encountered? (and should I explain its due to legacy compatibility reasons that the method wasn't just updated, or just leave that out)?

ccoVeille · 2025-11-22T11:03:41Z

@ccoVeille Do you think I should update the method comment

I think so, yes. Good idea

Copilot

Pull request overview

Refactors WatchConfig initialization to fail fast by panicking (instead of calling os.Exit inside a goroutine) when fsnotify watcher setup cannot be completed, aligning with issue #2095’s request to make failure behavior recoverable by callers.

Changes:

Moves fsnotify watcher creation and directory registration out of the goroutine so initialization failures occur synchronously.
Replaces os.Exit(1) with panics on watcher creation failure, and adds panics on config-file resolution and watcher registration failures.
Updates WatchConfig doc comments to mention panic behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-09T13:10:16Z

viper.go

+	filename, err := v.getConfigFile()
+	if err != nil {
+		err = fmt.Errorf("failed to get config file: %s", err)
+		v.logger.Error(err.Error())


These new panic-on-init-failure branches are not covered by existing WatchConfig tests. Please add a test that asserts WatchConfig panics on setup failure (e.g., getConfigFile error or watcher.Add error) so this behavior remains stable across refactors.

Copilot · 2026-03-09T13:10:17Z

viper.go

+		err = fmt.Errorf("failed to add config dir to watcher: %s", err)
+		v.logger.Error(err.Error())


Use %w instead of %s in fmt.Errorf here so the panicked error preserves the underlying failure for errors.Is/As checks.

Copilot · 2026-03-09T13:10:17Z

viper.go

+// If there is an error watching the config file, WatchConfig will panic.
 func WatchConfig() { v.WatchConfig() }

 // WatchConfig starts watching a config file for changes.
+// If there is an error watching the config file, WatchConfig will panic.


This global WatchConfig comment says the function will panic on any error "watching" the config file, but only setup errors panic; runtime watch/read errors are logged. Consider rewording to explicitly describe panic-on-setup-failure behavior.

Suggested change

// If there is an error watching the config file, WatchConfig will panic.

func WatchConfig() { v.WatchConfig() }

// WatchConfig starts watching a config file for changes.

// If there is an error watching the config file, WatchConfig will panic.

// It panics if setting up the watcher fails (for example, when creating the watcher,

// resolving the config file, or adding the config directory); subsequent watch or read

// errors are logged.

func WatchConfig() { v.WatchConfig() }

// WatchConfig starts watching a config file for changes.

// It panics if setting up the watcher fails (for example, when creating the watcher,

// resolving the config file, or adding the config directory); subsequent watch or read

// errors are logged.

Copilot · 2026-03-09T13:10:17Z

viper.go

+// If there is an error watching the config file, WatchConfig will panic.
 func WatchConfig() { v.WatchConfig() }

 // WatchConfig starts watching a config file for changes.
+// If there is an error watching the config file, WatchConfig will panic.


The doc comment says WatchConfig will panic "if there is an error watching the config file", but the implementation only panics on setup failures (NewWatcher/getConfigFile/Add). Runtime watcher errors and ReadInConfig errors are only logged. Please adjust the wording to match the actual behavior (e.g., panic on initialization/setup failure).

Suggested change

// If there is an error watching the config file, WatchConfig will panic.

func WatchConfig() { v.WatchConfig() }

// WatchConfig starts watching a config file for changes.

// If there is an error watching the config file, WatchConfig will panic.

// WatchConfig panics if it fails to initialize watching the config file (for example, creating the watcher or adding the config directory).

func WatchConfig() { v.WatchConfig() }

// WatchConfig starts watching a config file for changes.

// WatchConfig panics if it fails to initialize watching the config file (for example, creating the watcher or adding the config directory).

Copilot · 2026-03-09T13:10:17Z

viper.go

+	watcher, err := fsnotify.NewWatcher()
+	if err != nil {
+		err = fmt.Errorf("failed to create watcher: %s", err)
+		v.logger.Error(err.Error())
+		panic(err)


Use error wrapping with %w (instead of %s) when building the panic error so callers can inspect the root cause via errors.Is/As. This repo already uses %w in similar logger.Error(fmt.Errorf(...).Error()) patterns.

Copilot · 2026-03-09T13:10:18Z

viper.go

+	}
+
+	configFile := filepath.Clean(filename)
+	configDir, _ := filepath.Split(configFile)


filepath.Split can yield an empty configDir when the config file path has no directory component (e.g. "config.yaml"), which makes watcher.Add("") fail and now panic. Consider using filepath.Dir and normalizing empty to "." (or otherwise ensuring a valid directory path) before calling watcher.Add.

Suggested change

configDir, _ := filepath.Split(configFile)

configDir := filepath.Dir(configFile)

if configDir == "" {

configDir = "."

}

Copilot · 2026-03-09T13:10:18Z

viper.go

+		err = fmt.Errorf("failed to get config file: %s", err)
+		v.logger.Error(err.Error())


Use %w instead of %s in fmt.Errorf here so the returned/panicked error wraps the underlying cause for errors.Is/As checks.

sagikazarmark · 2026-03-09T13:16:47Z

Thanks @deefdragon

I reviewed the Copilot comments and they seem to be valid (even though some of them are probably for pre-existing issues, they are highlighted because they are in the diff)

ccoVeille reviewed Nov 22, 2025

View reviewed changes

Make errors consistent between the logged error and the error in the …

ed1917e

…panic

Update comment for WatchConfig to denote it may panic

f86aae5

sagikazarmark requested a review from Copilot March 9, 2026 13:05

Copilot started reviewing on behalf of sagikazarmark March 9, 2026 13:06 View session

Copilot AI reviewed Mar 9, 2026

View reviewed changes

		err = fmt.Errorf("failed to add config dir to watcher: %s", err)
		v.logger.Error(err.Error())

-	configDir, _ := filepath.Split(configFile)
+	configDir := filepath.Dir(configFile)
+	if configDir == "" {
+		configDir = "."
+	}

		err = fmt.Errorf("failed to get config file: %s", err)
		v.logger.Error(err.Error())

Conversation

deefdragon commented Nov 21, 2025

Uh oh!

CLAassistant commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ccoVeille Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deefdragon Nov 22, 2025

Choose a reason for hiding this comment

Uh oh!

ccoVeille Nov 22, 2025

Choose a reason for hiding this comment

Uh oh!

deefdragon commented Nov 22, 2025

Uh oh!

ccoVeille commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Mar 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Nov 21, 2025 •

edited

Loading

ccoVeille Nov 22, 2025 •

edited

Loading

ccoVeille commented Nov 22, 2025 •

edited

Loading