refactor(config): publish config singletons through atomic pointers

The three configuration singletons (C_globalConfig, C_botUserConfig, C_botNodeConfig) were plain variables: LoadGlobalConfig and friends assigned them from the goroutine handling a settings update, while bot pipes, the node tracker and the HTTP handlers read them from their own goroutines. That is an unsynchronized read of a concurrently written variable — a data race the race detector reports, and one that already existed before any hot-reload work because /api/webhook/add reloads the configuration while the bots run. Replace them with atomic.Pointer values behind Current(), BotUsers() and BotNodes(). Each reload builds a fresh value and publishes it atomically, so a reader either sees the previous configuration or the new one, never a partial one. Callers read through the accessor on every use instead of caching it. Two spots that read several fields of one guard now snapshot once per call, so a reload cannot split a combined check mid-flight: - qq_napcat.handleNapcatEvent, which evaluates the debug guards per event - the komari task-echo guard, now behind taskEchoEnabled() The NapCat HTTP methods keep logging on showNapcatAction alone (without requiring debugMode), matching their existing behaviour; that inconsistency with the WebSocket path is preserved, not introduced, and is called out in actionLogEnabled. Also adds TestConcurrentReloadAndRead, which drives every accessor from four reader goroutines while two writers reload the configuration, and sanitises the member IDs used as test fixtures in config/settings_test.go. Co-Authored-By: Claude Code <noreply@anthropic.com>
2026-09-28 22:47:37 +08:00
parent 2d5cf39151
commit cb2df5076d
16 changed files with 314 additions and 77 deletions
@@ -0,0 +1,164 @@
+package config
+
+import (
+	"sync"
+	"testing"
+
+	"nukumizu-backend/global"
+)
+
+// TestConcurrentReloadAndRead drives every configuration accessor from reader
+// goroutines while UpdateSettings and SaveBotNodeConfig replace the in-memory
+// configurations underneath them. Run with -race to check that the swap is
+// race-free: before the singletons were published through atomic pointers this
+// pattern was an unsynchronized read of a variable written by LoadGlobalConfig
+// and friends, which the race detector reports.
+//
+// The readers call the accessors the way production code does — take the value
+// and use it immediately, never store it — because that is what keeps a reader
+// pinned to one complete version of the configuration.
+func TestConcurrentReloadAndRead(t *testing.T) {
+	writeTempConfig(t, &global.ConfigPath.Global, `{
+    "system": {
+        "debugMode": true,
+        "listenPort": "8080"
+    },
+    "webhook": {
+        "endpoints": {
+            "example": { "enabled": true, "token": "t", "notifyPipes": ["ntfy"] }
+        }
+    },
+    "controllerMessage": {
+        "BOT_STARTED": "hello"
+    }
+}`)
+	writeTempConfig(t, &global.ConfigPath.BotUserConfig, `{
+    "qq(napcat)": {
+        "admins": { "1": { "event_reply": true } },
+        "trustedGroups": { "2": { "event_status_notify": true } }
+    }
+}`)
+	writeTempConfig(t, &global.ConfigPath.BotNodeConfig, `{
+    "node-1": { "enableStatusNotify": true }
+}`)
+
+	// Seed every singleton so the readers start from a loaded configuration
+	// rather than racing the first store.
+	if _, err := LoadGlobalConfig(global.ConfigPath.Global); err != nil {
+		t.Fatalf("LoadGlobalConfig: %v", err)
+	}
+	if _, err := LoadBotUserConfig(global.ConfigPath.BotUserConfig); err != nil {
+		t.Fatalf("LoadBotUserConfig: %v", err)
+	}
+	if err := LoadBotNodeConfig(global.ConfigPath.BotNodeConfig); err != nil {
+		t.Fatalf("LoadBotNodeConfig: %v", err)
+	}
+
+	const readers = 4
+	const rounds = 40
+
+	var readersWg, writersWg sync.WaitGroup
+	stop := make(chan struct{})
+
+	for i := 0; i < readers; i++ {
+		readersWg.Add(1)
+		go func() {
+			defer readersWg.Done()
+			for {
+				select {
+				case <-stop:
+					return
+				default:
+				}
+				if cfg := Current(); cfg != nil {
+					_ = cfg.System.DebugMode
+					_ = cfg.System.ListenPort
+					_ = cfg.ControllerMessage.BotStarted
+					_ = cfg.Webhook.Endpoints
+				}
+				if users := BotUsers(); users != nil {
+					_ = users.QQ.Admins.IDs()
+					_ = users.QQ.TrustedGroups.IDs()
+				}
+				_ = BotNodes()
+				_ = IsDebugMode()
+				_ = NodeStatusNotifyEnabled("node-1")
+				_ = WebhookEndpoints()
+				_, _ = GetWebhookEndpoint("example")
+			}
+		}()
+	}
+
+	// Writer: reloads the global and bot user configurations, and rewrites the
+	// node registry through UpdateSettings so its reload runs too.
+	writersWg.Add(1)
+	go func() {
+		defer writersWg.Done()
+		for i := 0; i < rounds; i++ {
+			enabled := i%2 == 0
+			patch := map[string]interface{}{
+				"system": map[string]interface{}{"debugMode": enabled},
+				"controllerMessage": map[string]interface{}{
+					"BOT_STARTED": "hello",
+				},
+			}
+			if err := UpdateSettings(SettingGlobal, patch); err != nil {
+				t.Errorf("UpdateSettings(global): %v", err)
+				return
+			}
+			if err := UpdateSettings(SettingBotUserConfig, map[string]interface{}{
+				"qq(napcat)": map[string]interface{}{
+					"admins": map[string]interface{}{
+						"1": map[string]interface{}{"event_reply": enabled},
+					},
+				},
+			}); err != nil {
+				t.Errorf("UpdateSettings(bot_user_config): %v", err)
+				return
+			}
+			if err := UpdateSettings(SettingBotNodeConfig, map[string]interface{}{
+				"node-2": map[string]interface{}{"enableStatusNotify": enabled},
+			}); err != nil {
+				t.Errorf("UpdateSettings(bot_node_config): %v", err)
+				return
+			}
+		}
+	}()
+
+	// Second writer: the node tracker's background save, which shares the same
+	// read-modify-write lock as the admin edits above.
+	writersWg.Add(1)
+	go func() {
+		defer writersWg.Done()
+		for i := 0; i < rounds; i++ {
+			if err := SaveBotNodeConfig(global.ConfigPath.BotNodeConfig, []string{"node-1", "node-2"}); err != nil {
+				t.Errorf("SaveBotNodeConfig: %v", err)
+				return
+			}
+		}
+	}()
+
+	// Let the writers finish, then release the readers. Waiting on the readers
+	// first would deadlock: they only return once stop is closed.
+	writersWg.Wait()
+	close(stop)
+	readersWg.Wait()
+
+	// The last write must be visible: the accessors are not allowed to serve a
+	// stale configuration once UpdateSettings has returned.
+	if err := UpdateSettings(SettingGlobal, map[string]interface{}{
+		"system": map[string]interface{}{"debugMode": true},
+	}); err != nil {
+		t.Fatalf("final UpdateSettings(global): %v", err)
+	}
+	cfg := Current()
+	if cfg == nil {
+		t.Fatal("Current() is nil after a successful reload")
+	}
+	if !cfg.System.DebugMode {
+		t.Error("Current() did not observe the reloaded debugMode")
+	}
+	if cfg.System.ListenPort != "8080" {
+		t.Errorf("reload dropped an untouched sibling: listenPort = %q", cfg.System.ListenPort)
+	}
+}
@@ -17,7 +17,7 @@ func LoadBotNodeConfig(configPath string) error {
 	data, err := os.ReadFile(configPath)
 	if err != nil {
 		if os.IsNotExist(err) {
-			C_botNodeConfig = cfg
+			botNodeConfig.Store(&cfg)
 			return nil
 		}
 		return fmt.Errorf("failed to read bot node config file: %w", err)
@@ -27,19 +27,17 @@ func LoadBotNodeConfig(configPath string) error {
 			return fmt.Errorf("failed to parse bot node config file: %w", err)
 		}
 	}
-	C_botNodeConfig = cfg
+	botNodeConfig.Store(&cfg)
 	return nil
 }

 // NodeStatusNotifyEnabled reports whether the node identified by uuid should
 // broadcast status-change notifications, per bot_node_config.json.
 // enableStatusNotify defaults to true: a node notifies unless its entry
-// explicitly sets the flag to false.
+// explicitly sets the flag to false. A missing configuration (nil map) yields
+// the same default.
 func NodeStatusNotifyEnabled(uuid string) bool {
-	if C_botNodeConfig == nil {
-		return true
-	}
-	opts, ok := C_botNodeConfig[uuid]
+	opts, ok := BotNodes()[uuid]
 	if !ok || opts.EnableStatusNotify == nil {
 		return true
 	}
@@ -146,7 +144,7 @@ func LoadGlobalConfig(configPath string) (*Config, error) {
 		cfg.ControllerMessage.ServerExecuteResult = "Command execute result:\nServer Name: {{ serverName }}\nCommand: {{ command }}\n***Result***\n\n{{ result }}\n\n************\nTime: {{ time }}"
 	}

-	C_globalConfig = &cfg
+	globalConfig.Store(&cfg)
 	return &cfg, nil
 }

@@ -162,7 +160,7 @@ func LoadBotUserConfig(configPath string) (*BotUserConfig, error) {
 	if err := json.Unmarshal(data, &cfg); err != nil {
 		return nil, fmt.Errorf("failed to parse bot user config file: %w", err)
 	}
-	C_botUserConfig = &cfg
+	botUserConfig.Store(&cfg)
 	return &cfg, nil
 }

@@ -213,8 +211,9 @@ func SaveBotNodeConfig(configPath string, uuids []string) error {

 // IsDebugMode returns whether debug mode is enabled.
 func IsDebugMode() bool {
-	if C_globalConfig == nil {
+	cfg := Current()
+	if cfg == nil {
 		return false
 	}
-	return C_globalConfig.System.DebugMode
+	return cfg.System.DebugMode
 }
@@ -1,6 +1,9 @@
 package config

-import "sort"
+import (
+	"sort"
+	"sync/atomic"
+)

 // SystemConfig holds system-level configuration.
 type SystemConfig struct {
@@ -136,10 +139,11 @@ type WebhookReceiverConfig struct {
 // GetWebhookEndpoint returns the incoming webhook endpoint registered under the
 // given name, and whether such an endpoint exists.
 func GetWebhookEndpoint(name string) (WebhookEndpointConfig, bool) {
-	if C_globalConfig == nil {
+	cfg := Current()
+	if cfg == nil {
 		return WebhookEndpointConfig{}, false
 	}
-	endpoint, ok := C_globalConfig.Webhook.Endpoints[name]
+	endpoint, ok := cfg.Webhook.Endpoints[name]
 	return endpoint, ok
 }

@@ -165,7 +169,19 @@ type Config struct {
 	DBPath            string                  `json:"dbPath"`
 }

-var C_globalConfig *Config
+// globalConfig holds the configuration currently in effect. It is replaced as a
+// whole by LoadGlobalConfig — and therefore by every settings update — and never
+// mutated in place, so a reader that loads the pointer always observes a fully
+// initialized Config. Read it through Current rather than caching the result: a
+// cached pointer stops tracking reloads.
+var globalConfig atomic.Pointer[Config]
+
+// Current returns the configuration currently in effect, or nil before the
+// first successful LoadGlobalConfig. It is safe to call from any goroutine, and
+// must be called on every use rather than stored, so the caller sees reloads.
+func Current() *Config {
+	return globalConfig.Load()
+}

 // BotUserOptions holds per-member options stored in bot_user_config.json.
 type BotUserOptions struct {
@@ -220,7 +236,16 @@ type BotUserConfig struct {
 	Telegram BotUser_TelegramConfig `json:"telegram"`
 }

-var C_botUserConfig *BotUserConfig
+// botUserConfig mirrors bot_user_config.json the same way globalConfig mirrors
+// config.json: replaced wholesale on reload and read through BotUsers.
+var botUserConfig atomic.Pointer[BotUserConfig]
+
+// BotUsers returns the bot user configuration currently in effect, or nil
+// before the first successful LoadBotUserConfig. Like Current it must be called
+// on every use rather than stored.
+func BotUsers() *BotUserConfig {
+	return botUserConfig.Load()
+}

 // BotNodeOptions holds per-node options stored in bot_node_config.json. The
 // file is auto-populated by the node tracker for every node Komari reports;
@@ -238,6 +263,18 @@ type BotNodeOptions struct {
 // options.
 type BotNodeMembers map[string]BotNodeOptions

-// C_botNodeConfig is the global singleton mirroring bot_node_config.json,
-// populated by LoadBotNodeConfig.
-var C_botNodeConfig BotNodeMembers
+// botNodeConfig mirrors bot_node_config.json, populated by LoadBotNodeConfig.
+// The map is rebuilt rather than mutated on every load, so the pointer can be
+// swapped atomically; read it through BotNodes.
+var botNodeConfig atomic.Pointer[BotNodeMembers]
+
+// BotNodes returns the per-node options currently in effect, or nil before the
+// first LoadBotNodeConfig. Like Current it must be called on every use rather
+// than stored.
+func BotNodes() BotNodeMembers {
+	nodes := botNodeConfig.Load()
+	if nodes == nil {
+		return nil
+	}
+	return *nodes
+}
@@ -37,10 +37,11 @@ var webhookEndpointFields = map[string]func(interface{}) bool{
 // configuration.
 func WebhookEndpoints() map[string]WebhookEndpointConfig {
 	endpoints := map[string]WebhookEndpointConfig{}
-	if C_globalConfig == nil {
+	cfg := Current()
+	if cfg == nil {
 		return endpoints
 	}
-	for name, endpoint := range C_globalConfig.Webhook.Endpoints {
+	for name, endpoint := range cfg.Webhook.Endpoints {
 		endpoints[name] = endpoint
 	}
 	return endpoints
@@ -107,14 +108,15 @@ func DeleteWebhookEndpoint(name string) error {
 }

 // webhookEndpoint returns the named endpoint held by the loaded configuration.
-// No lock is needed to read it: a reload replaces the whole configuration
-// rather than mutating it in place, and the value is read from whichever
-// version is current.
+// No extra lock is needed to read it: a reload replaces the whole configuration
+// rather than mutating it in place, and Current publishes the replacement
+// atomically, so the value read is always from one complete version.
 func webhookEndpoint(name string) (WebhookEndpointConfig, bool) {
-	if C_globalConfig == nil {
+	cfg := Current()
+	if cfg == nil {
 		return WebhookEndpointConfig{}, false
 	}
-	endpoint, exists := C_globalConfig.Webhook.Endpoints[name]
+	endpoint, exists := cfg.Webhook.Endpoints[name]
 	return endpoint, exists
 }