fix(config): prevent race condition with atomic write and skipNextReload flag

commit: 61db896402f5d05325d66951cc9b4a43b919dc4f
author: brice · 9mo ago · 2026-01-24 14:18 UTC
parent: 1960daf
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

3 files · +149 -59

M internal/infrastructure/config/loader.go +17 -6


@@ -17,11 +17,12 @@ import (
17	17
18	18		// Manager handles configuration loading, watching, and reloading.
19	19		type Manager struct {
20		-	config *Config
21		-	viper *viper.Viper
22		-	mu sync.RWMutex
23		-	callbacks []func(*Config)
24		-	watching bool
	20	+	config *Config
	21	+	viper *viper.Viper
	22	+	mu sync.RWMutex
	23	+	callbacks []func(*Config)
	24	+	watching bool
	25	+	skipNextReload bool // Set by Save() to prevent fsnotify reload from overwriting in-memory config
25	26		}
26	27
27	28		// NewManager creates a new configuration manager.

@@ -283,12 +284,22 @@ func (m Manager) Save(cfg Config) error {
283	284		}
284	285		}
285	286
	287	+	// Set flag BEFORE writing to prevent fsnotify-triggered reload from
	288	+	// overwriting our in-memory config with stale viper cache
	289	+	m.skipNextReload = true
	290	+
286	291		if err := WriteConfigOrdered(cfg, configFile); err != nil {
	292	+	m.skipNextReload = false // Reset on error
287	293		return fmt.Errorf("failed to write config: %w", err)
288	294		}
289	295
290		-	// Reload config from disk to sync Viper's internal state
	296	+	// Update in-memory config immediately (don't wait for file watcher)
	297	+	// This ensures subsequent Get() calls return the new values
	298	+	m.config = cfg
	299	+
	300	+	// Also reload from disk to sync Viper's internal state (if not watching)
291	301		if !m.watching {
	302	+	m.skipNextReload = false // Clear flag since we're doing manual reload
292	303		if err := m.reload(); err != nil {
293	304		return err
294	305		}

 // Manager handles configuration loading, watching, and reloading.
 type Manager struct {
-	config    *Config
+	config         *Config
-	viper     *viper.Viper
+	viper          *viper.Viper
-	mu        sync.RWMutex
+	mu             sync.RWMutex
-	callbacks []func(*Config)
+	callbacks      []func(*Config)
-	watching  bool
+	watching       bool
+	skipNextReload bool // Set by Save() to prevent fsnotify reload from overwriting in-memory config
 }
 // NewManager creates a new configuration manager.
 		}
 	}
+	// Set flag BEFORE writing to prevent fsnotify-triggered reload from
+	// overwriting our in-memory config with stale viper cache
+	m.skipNextReload = true
 	if err := WriteConfigOrdered(cfg, configFile); err != nil {
+		m.skipNextReload = false // Reset on error
 		return fmt.Errorf("failed to write config: %w", err)
 	}
-	// Reload config from disk to sync Viper's internal state
+	// Update in-memory config immediately (don't wait for file watcher)
+	// This ensures subsequent Get() calls return the new values
+	m.config = cfg
+	// Also reload from disk to sync Viper's internal state (if not watching)
 	if !m.watching {
+		m.skipNextReload = false // Clear flag since we're doing manual reload
 		if err := m.reload(); err != nil {
 			return err
 		}

M internal/infrastructure/config/watcher.go +35 -14


@@ -2,8 +2,8 @@ package config
2	2
3	3		import (
4	4		"fmt"
5		-	"os"
6	5
	6	+	"github.com/bnema/dumber/internal/logging"
7	7		"github.com/fsnotify/fsnotify"
8	8		)
9	9

@@ -17,29 +17,50 @@ func (m *Manager) Watch() error {
17	17		}
18	18
19	19		m.viper.WatchConfig()
20		-	m.viper.OnConfigChange(func(_ fsnotify.Event) {
21		-	// Reload config
22		-	if err := m.reload(); err != nil {
23		-	fmt.Fprintf(os.Stderr, "Warning: failed to reload config: %v\n", err)
	20	+	m.viper.OnConfigChange(func(e fsnotify.Event) {
	21	+	log := logging.NewFromEnv()
	22	+	log.Debug().Str("op", e.Op.String()).Str("file", e.Name).Msg("fsnotify config change detected")
	23	+
	24	+	// Acquire write lock before reload (reload modifies m.config)
	25	+	m.mu.Lock()
	26	+
	27	+	// Skip reload if this was triggered by our own Save() - the in-memory
	28	+	// config is already correct and viper may have stale cached data
	29	+	if m.skipNextReload {
	30	+	log.Debug().Msg("skipping reload (triggered by own Save)")
	31	+	m.skipNextReload = false
	32	+	// Still need to sync viper's internal state with the file we just wrote
	33	+	_ = m.viper.ReadInConfig()
	34	+	m.notifyCallbacksLocked()
24	35		return
25	36		}
26	37
27		-	// Notify callbacks
28		-	m.mu.RLock()
29		-	config := m.config
30		-	callbacks := make([]func(*Config), len(m.callbacks))
31		-	copy(callbacks, m.callbacks)
32		-	m.mu.RUnlock()
33		-
34		-	for _, callback := range callbacks {
35		-	callback(config)
	38	+	log.Debug().Msg("reloading config from external change")
	39	+	if err := m.reload(); err != nil {
	40	+	m.mu.Unlock()
	41	+	log.Warn().Err(err).Msg("failed to reload config")
	42	+	return
36	43		}
	44	+	m.notifyCallbacksLocked()
37	45		})
38	46
39	47		m.watching = true
40	48		return nil
41	49		}
42	50
	51	+	// notifyCallbacksLocked copies callbacks and config, releases lock, then notifies.
	52	+	// Must be called with m.mu held for write. Releases the lock before calling callbacks.
	53	+	func (m *Manager) notifyCallbacksLocked() {
	54	+	config := m.config
	55	+	callbacks := make([]func(*Config), len(m.callbacks))
	56	+	copy(callbacks, m.callbacks)
	57	+	m.mu.Unlock()
	58	+
	59	+	for _, callback := range callbacks {
	60	+	callback(config)
	61	+	}
	62	+	}
	63	+
43	64		// OnConfigChange registers a callback function to be called when config changes.
44	65		func (m Manager) OnConfigChange(callback func(Config)) {
45	66		m.mu.Lock()


@@ -2,8 +2,8 @@ package config
2			2
3		import (	3		import (
4		"fmt"	4		"fmt"
5	-	"os"
6			5
			6	+	"github.com/bnema/dumber/internal/logging"
7		"github.com/fsnotify/fsnotify"	7		"github.com/fsnotify/fsnotify"
8		)	8		)
9			9

@@ -17,29 +17,50 @@ func (m *Manager) Watch() error {
17		}	17		}
18			18
19		m.viper.WatchConfig()	19		m.viper.WatchConfig()
20	-	m.viper.OnConfigChange(func(_ fsnotify.Event) {	20	+	m.viper.OnConfigChange(func(e fsnotify.Event) {
21	-	// Reload config	21	+	log := logging.NewFromEnv()
22	-	if err := m.reload(); err != nil {	22	+	log.Debug().Str("op", e.Op.String()).Str("file", e.Name).Msg("fsnotify config change detected")
23	-	fmt.Fprintf(os.Stderr, "Warning: failed to reload config: %v\n", err)	23	+
			24	+	// Acquire write lock before reload (reload modifies m.config)
			25	+	m.mu.Lock()
			26	+
			27	+	// Skip reload if this was triggered by our own Save() - the in-memory
			28	+	// config is already correct and viper may have stale cached data
			29	+	if m.skipNextReload {
			30	+	log.Debug().Msg("skipping reload (triggered by own Save)")
			31	+	m.skipNextReload = false
			32	+	// Still need to sync viper's internal state with the file we just wrote
			33	+	_ = m.viper.ReadInConfig()
			34	+	m.notifyCallbacksLocked()
24		return	35		return
25		}	36		}
26			37
27	-	// Notify callbacks	38	+	log.Debug().Msg("reloading config from external change")
28	-	m.mu.RLock()	39	+	if err := m.reload(); err != nil {
29	-	config := m.config	40	+	m.mu.Unlock()
30	-	callbacks := make([]func(*Config), len(m.callbacks))	41	+	log.Warn().Err(err).Msg("failed to reload config")
31	-	copy(callbacks, m.callbacks)	42	+	return
32	-	m.mu.RUnlock()
33	-
34	-	for _, callback := range callbacks {
35	-	callback(config)
36		}	43		}
			44	+	m.notifyCallbacksLocked()
37		})	45		})
38			46
39		m.watching = true	47		m.watching = true
40		return nil	48		return nil
41		}	49		}
42			50
			51	+	// notifyCallbacksLocked copies callbacks and config, releases lock, then notifies.
			52	+	// Must be called with m.mu held for write. Releases the lock before calling callbacks.
			53	+	func (m *Manager) notifyCallbacksLocked() {
			54	+	config := m.config
			55	+	callbacks := make([]func(*Config), len(m.callbacks))
			56	+	copy(callbacks, m.callbacks)
			57	+	m.mu.Unlock()
			58	+
			59	+	for _, callback := range callbacks {
			60	+	callback(config)
			61	+	}
			62	+	}
			63	+
43		// OnConfigChange registers a callback function to be called when config changes.	64		// OnConfigChange registers a callback function to be called when config changes.
44		func (m Manager) OnConfigChange(callback func(Config)) {	65		func (m Manager) OnConfigChange(callback func(Config)) {
45		m.mu.Lock()	66		m.mu.Lock()

M internal/infrastructure/config/writer.go +97 -39


@@ -4,99 +4,158 @@ import (
4	4		"bytes"
5	5		"fmt"
6	6		"os"
	7	+	"path/filepath"
7	8		"regexp"
8	9		"sort"
9	10		"strings"
10	11
	12	+	"github.com/bnema/dumber/internal/logging"
11	13		"github.com/pelletier/go-toml/v2"
12	14		)
13	15
14	16		// WriteConfigOrdered writes the configuration to disk with consistent ordering.
15		-	// - Struct fields are written in definition order (go-toml v2 behavior)
16		-	// - TOML sections are sorted alphabetically within each parent for deterministic output
	17	+	// Uses atomic write (temp file + rename) to prevent race conditions with file watchers.
17	18		func WriteConfigOrdered(cfg *Config, path string) error {
18	19		if cfg == nil {
19	20		return fmt.Errorf("config is nil")
20	21		}
21	22
	23	+	content, err := encodeConfigToTOML(cfg)
	24	+	if err != nil {
	25	+	return err
	26	+	}
	27	+
	28	+	return atomicWriteFile(path, content, filePerm)
	29	+	}
	30	+
	31	+	// encodeConfigToTOML encodes the config struct to sorted TOML content.
	32	+	func encodeConfigToTOML(cfg *Config) (string, error) {
22	33		var buf bytes.Buffer
23	34		enc := toml.NewEncoder(&buf)
24	35		enc.SetIndentTables(true)
25	36
26	37		if err := enc.Encode(cfg); err != nil {
27		-	return fmt.Errorf("failed to encode config: %w", err)
	38	+	return "", fmt.Errorf("failed to encode config: %w", err)
28	39		}
29	40
30		-	// Post-process to sort TOML sections alphabetically
31		-	sorted := sortTOMLSections(buf.String())
	41	+	return sortTOMLSections(buf.String()), nil
	42	+	}
	43	+
	44	+	// atomicWriteFile writes content to a file atomically using temp file + rename.
	45	+	// This ensures file watchers only see complete content, not partial writes.
	46	+	func atomicWriteFile(path, content string, perm os.FileMode) error {
	47	+	dir := filepath.Dir(path)
	48	+	tmpFile, err := os.CreateTemp(dir, ".config-*.tmp")
	49	+	if err != nil {
	50	+	return fmt.Errorf("failed to create temp file: %w", err)
	51	+	}
	52	+	tmpPath := tmpFile.Name()
	53	+
	54	+	// Cleanup on error
	55	+	success := false
	56	+	defer func() {
	57	+	if !success {
	58	+	if removeErr := os.Remove(tmpPath); removeErr != nil {
	59	+	log := logging.NewFromEnv()
	60	+	log.Warn().Err(removeErr).Str("path", tmpPath).Msg("failed to cleanup temp config file")
	61	+	}
	62	+	}
	63	+	}()
32	64
33		-	if err := os.WriteFile(path, []byte(sorted), filePerm); err != nil {
34		-	return fmt.Errorf("failed to write config file: %w", err)
	65	+	if err := writeAndSync(tmpFile, content); err != nil {
	66	+	return err
35	67		}
36	68
	69	+	if err := os.Chmod(tmpPath, perm); err != nil {
	70	+	return fmt.Errorf("failed to set file permissions: %w", err)
	71	+	}
	72	+
	73	+	if err := os.Rename(tmpPath, path); err != nil {
	74	+	return fmt.Errorf("failed to rename temp file: %w", err)
	75	+	}
	76	+
	77	+	success = true
37	78		return nil
38	79		}
39	80
40		-	// sortTOMLSections sorts TOML content so sections are in alphabetical order.
41		-	// This handles both top-level sections and indented nested sections.
	81	+	// writeAndSync writes content to file and syncs to disk.
	82	+	func writeAndSync(f *os.File, content string) (err error) {
	83	+	// Ensure file is closed, capturing close error if no prior error
	84	+	defer func() {
	85	+	closeErr := f.Close()
	86	+	if err == nil && closeErr != nil {
	87	+	err = fmt.Errorf("failed to close file: %w", closeErr)
	88	+	}
	89	+	}()
	90	+
	91	+	if _, err = f.WriteString(content); err != nil {
	92	+	return fmt.Errorf("failed to write file: %w", err)
	93	+	}
	94	+
	95	+	if err = f.Sync(); err != nil {
	96	+	return fmt.Errorf("failed to sync file: %w", err)
	97	+	}
	98	+
	99	+	return nil
	100	+	}
	101	+
	102	+	// tomlSection represents a parsed TOML section with its header and content lines.
	103	+	type tomlSection struct {
	104	+	header string // Section name without brackets (e.g., "workspace.shortcuts")
	105	+	lines []string // All lines belonging to this section, including the header
	106	+	}
	107	+
	108	+	// sortTOMLSections sorts TOML content so sections appear in alphabetical order.
42	109		func sortTOMLSections(content string) string {
43	110		lines := strings.Split(content, "\n")
	111	+	preamble, sections := parseTOMLSections(lines)
44	112
45		-	// Parse into sections
46		-	type section struct {
47		-	header string // e.g., "appearance" or "workspace.pane_mode.actions.cancel"
48		-	lines []string // lines belonging to this section (including header)
49		-	}
	113	+	sort.Slice(sections, func(i, j int) bool {
	114	+	return sections[i].header < sections[j].header
	115	+	})
50	116
51		-	var sections []section
52		-	var currentSection *section
53		-	var preamble []string // lines before first section
	117	+	return buildTOMLOutput(preamble, sections)
	118	+	}
54	119
55		-	// Match section headers with optional leading whitespace (for indented sub-tables)
	120	+	// parseTOMLSections parses TOML lines into preamble (top-level keys) and sections.
	121	+	func parseTOMLSections(lines []string) (preamble []string, sections []tomlSection) {
56	122		sectionRegex := regexp.MustCompile(`^(\s)\[([^\]]+)\]\s$`)
	123	+	var current *tomlSection
57	124
58	125		for _, line := range lines {
59	126		if match := sectionRegex.FindStringSubmatch(line); match != nil {
60		-	// New section found
61		-	if currentSection != nil {
62		-	sections = append(sections, *currentSection)
	127	+	if current != nil {
	128	+	sections = append(sections, *current)
63	129		}
64		-	currentSection = &section{
65		-	header: match[2], // Just the section name, without brackets or indent
	130	+	current = &tomlSection{
	131	+	header: match[2],
66	132		lines: []string{line},
67	133		}
68		-	} else if currentSection != nil {
69		-	currentSection.lines = append(currentSection.lines, line)
	134	+	} else if current != nil {
	135	+	current.lines = append(current.lines, line)
70	136		} else {
71		-	// Before any section (top-level keys)
72	137		preamble = append(preamble, line)
73	138		}
74	139		}
75	140
76		-	// Don't forget the last section
77		-	if currentSection != nil {
78		-	sections = append(sections, *currentSection)
	141	+	if current != nil {
	142	+	sections = append(sections, *current)
79	143		}
80	144
81		-	// Sort sections alphabetically by header
82		-	sort.Slice(sections, func(i, j int) bool {
83		-	return sections[i].header < sections[j].header
84		-	})
	145	+	return preamble, sections
	146	+	}
85	147
86		-	// Rebuild content
	148	+	// buildTOMLOutput reconstructs TOML content from preamble and sorted sections.
	149	+	func buildTOMLOutput(preamble []string, sections []tomlSection) string {
87	150		var result strings.Builder
88	151
89		-	// Write preamble (top-level keys) first
90	152		for _, line := range preamble {
91	153		result.WriteString(line)
92	154		result.WriteString("\n")
93	155		}
94	156
95		-	// Write sorted sections
96	157		for i, sec := range sections {
97		-	// Add blank line before section (except first if preamble is empty)
98	158		if i > 0 \|\| len(preamble) > 0 {
99		-	// Check if previous content already ends with blank line
100	159		content := result.String()
101	160		if !strings.HasSuffix(content, "\n\n") && content != "" {
102	161		result.WriteString("\n")

@@ -109,7 +168,6 @@ func sortTOMLSections(content string) string {
109	168		}
110	169		}
111	170
112		-	// Trim trailing whitespace but ensure single newline at end
113	171		output := strings.TrimRight(result.String(), "\n")
114	172		if output != "" {
115	173		output += "\n"

 	"bytes"
 	"fmt"
 	"os"
+	"path/filepath"
 	"regexp"
 	"sort"
 	"strings"
+	"github.com/bnema/dumber/internal/logging"
 	"github.com/pelletier/go-toml/v2"
 )
 // WriteConfigOrdered writes the configuration to disk with consistent ordering.
-// - Struct fields are written in definition order (go-toml v2 behavior)
+// Uses atomic write (temp file + rename) to prevent race conditions with file watchers.
-// - TOML sections are sorted alphabetically within each parent for deterministic output
 func WriteConfigOrdered(cfg *Config, path string) error {
 	if cfg == nil {
 		return fmt.Errorf("config is nil")
 	}
+	content, err := encodeConfigToTOML(cfg)
+	if err != nil {
+		return err
+	}
+	return atomicWriteFile(path, content, filePerm)
+}
+// encodeConfigToTOML encodes the config struct to sorted TOML content.
+func encodeConfigToTOML(cfg *Config) (string, error) {
 	var buf bytes.Buffer
 	enc := toml.NewEncoder(&buf)
 	enc.SetIndentTables(true)
 	if err := enc.Encode(cfg); err != nil {
-		return fmt.Errorf("failed to encode config: %w", err)
+		return "", fmt.Errorf("failed to encode config: %w", err)
 	}
-	// Post-process to sort TOML sections alphabetically
+	return sortTOMLSections(buf.String()), nil
-	sorted := sortTOMLSections(buf.String())
+}
+// atomicWriteFile writes content to a file atomically using temp file + rename.
+// This ensures file watchers only see complete content, not partial writes.
+func atomicWriteFile(path, content string, perm os.FileMode) error {
+	dir := filepath.Dir(path)
+	tmpFile, err := os.CreateTemp(dir, ".config-*.tmp")
+	if err != nil {
+		return fmt.Errorf("failed to create temp file: %w", err)
+	}
+	tmpPath := tmpFile.Name()
+	// Cleanup on error
+	success := false
+	defer func() {
+		if !success {
+			if removeErr := os.Remove(tmpPath); removeErr != nil {
+				log := logging.NewFromEnv()
+				log.Warn().Err(removeErr).Str("path", tmpPath).Msg("failed to cleanup temp config file")
+			}
+		}
+	}()
-	if err := os.WriteFile(path, []byte(sorted), filePerm); err != nil {
+	if err := writeAndSync(tmpFile, content); err != nil {
-		return fmt.Errorf("failed to write config file: %w", err)
+		return err
 	}
+	if err := os.Chmod(tmpPath, perm); err != nil {
+		return fmt.Errorf("failed to set file permissions: %w", err)
+	}
+	if err := os.Rename(tmpPath, path); err != nil {
+		return fmt.Errorf("failed to rename temp file: %w", err)
+	}
+	success = true
 	return nil
 }
-// sortTOMLSections sorts TOML content so sections are in alphabetical order.
+// writeAndSync writes content to file and syncs to disk.
-// This handles both top-level sections and indented nested sections.
+func writeAndSync(f *os.File, content string) (err error) {
+	// Ensure file is closed, capturing close error if no prior error
+	defer func() {
+		closeErr := f.Close()
+		if err == nil && closeErr != nil {
+			err = fmt.Errorf("failed to close file: %w", closeErr)
+		}
+	}()
+	if _, err = f.WriteString(content); err != nil {
+		return fmt.Errorf("failed to write file: %w", err)
+	}
+	if err = f.Sync(); err != nil {
+		return fmt.Errorf("failed to sync file: %w", err)
+	}
+	return nil
+}
+// tomlSection represents a parsed TOML section with its header and content lines.
+type tomlSection struct {
+	header string   // Section name without brackets (e.g., "workspace.shortcuts")
+	lines  []string // All lines belonging to this section, including the header
+}
+// sortTOMLSections sorts TOML content so sections appear in alphabetical order.
 func sortTOMLSections(content string) string {
 	lines := strings.Split(content, "\n")
+	preamble, sections := parseTOMLSections(lines)
-	// Parse into sections
+	sort.Slice(sections, func(i, j int) bool {
-	type section struct {
+		return sections[i].header < sections[j].header
-		header string   // e.g., "appearance" or "workspace.pane_mode.actions.cancel"
+	})
-		lines  []string // lines belonging to this section (including header)
-	}
-	var sections []section
+	return buildTOMLOutput(preamble, sections)
-	var currentSection *section
+}
-	var preamble []string // lines before first section
-	// Match section headers with optional leading whitespace (for indented sub-tables)
+// parseTOMLSections parses TOML lines into preamble (top-level keys) and sections.
+func parseTOMLSections(lines []string) (preamble []string, sections []tomlSection) {
 	sectionRegex := regexp.MustCompile(`^(\s*)\[([^\]]+)\]\s*$`)
+	var current *tomlSection
 	for _, line := range lines {
 		if match := sectionRegex.FindStringSubmatch(line); match != nil {
-			// New section found
+			if current != nil {
-			if currentSection != nil {
+				sections = append(sections, *current)
-				sections = append(sections, *currentSection)
 			}
-			currentSection = &section{
+			current = &tomlSection{
-				header: match[2], // Just the section name, without brackets or indent
+				header: match[2],
 				lines:  []string{line},
 			}
-		} else if currentSection != nil {
+		} else if current != nil {
-			currentSection.lines = append(currentSection.lines, line)
+			current.lines = append(current.lines, line)
 		} else {
-			// Before any section (top-level keys)
 			preamble = append(preamble, line)
 		}
 	}
-	// Don't forget the last section
+	if current != nil {
-	if currentSection != nil {
+		sections = append(sections, *current)
-		sections = append(sections, *currentSection)
 	}
-	// Sort sections alphabetically by header
+	return preamble, sections
-	sort.Slice(sections, func(i, j int) bool {
+}
-		return sections[i].header < sections[j].header
-	})
-	// Rebuild content
+// buildTOMLOutput reconstructs TOML content from preamble and sorted sections.
+func buildTOMLOutput(preamble []string, sections []tomlSection) string {
 	var result strings.Builder
-	// Write preamble (top-level keys) first
 	for _, line := range preamble {
 		result.WriteString(line)
 		result.WriteString("\n")
 	}
-	// Write sorted sections
 	for i, sec := range sections {
-		// Add blank line before section (except first if preamble is empty)
 		if i > 0 || len(preamble) > 0 {
-			// Check if previous content already ends with blank line
 			content := result.String()
 			if !strings.HasSuffix(content, "\n\n") && content != "" {
 				result.WriteString("\n")
 		}
 	}
-	// Trim trailing whitespace but ensure single newline at end
 	output := strings.TrimRight(result.String(), "\n")
 	if output != "" {
 		output += "\n"