diff --git a/config/reload.go b/config/reload.go new file mode 100644 index 0000000..61bac53 --- /dev/null +++ b/config/reload.go @@ -0,0 +1,73 @@ +package config + +import ( + "fmt" + "sync" + + "nukumizu-backend/postLog" +) + +// reloadHooks are the callbacks run after the global configuration has been +// reloaded, i.e. after every settings update that touches config.json. +// +// They exist so that packages which already depend on config — the controller +// manager, the logger — can react to an update without config importing them, +// which would be an import cycle. Everything a hook needs is passed in. +var ( + reloadHooksMu sync.Mutex + reloadHooks []func(*Config) +) + +// OnReload registers a hook to run after every reload of the global +// configuration, receiving the configuration now in effect. Hooks run in +// registration order. +// +// Register once, at startup, before the first settings update can arrive: a +// hook registered later has already missed the updates that came before it, and +// the configuration it would have seen is not replayed. +// +// A hook runs on the goroutine serving /api/settings/set, so it must not block +// for long. It may be called concurrently by two overlapping updates. +func OnReload(hook func(*Config)) { + reloadHooksMu.Lock() + defer reloadHooksMu.Unlock() + reloadHooks = append(reloadHooks, hook) +} + +// notifyReload runs every registered hook with cfg. A nil cfg is the signal +// that the update touched one of the other settings files, which have no +// hook-visible reload, and it is ignored. +// +// A panicking hook is logged and skipped rather than allowed to unwind through +// UpdateSettings: by the time hooks run the new configuration is already on +// disk and published in memory, so reporting the write as failed would be a +// lie, and the hooks registered after the broken one must still run. +func notifyReload(cfg *Config) { + if cfg == nil { + return + } + + // Copy under the lock, then run outside it: a hook is free to register + // another hook without deadlocking. + reloadHooksMu.Lock() + hooks := make([]func(*Config), len(reloadHooks)) + copy(hooks, reloadHooks) + reloadHooksMu.Unlock() + + for _, hook := range hooks { + runReloadHook(hook, cfg) + } +} + +// runReloadHook runs one hook, isolating a panic to that hook. Recovering in a +// separate function rather than inline is deliberate: a deferred recover placed +// in the loop body would not run until notifyReload itself returned, which +// would abandon the remaining hooks. +func runReloadHook(hook func(*Config), cfg *Config) { + defer func() { + if r := recover(); r != nil { + postLog.Error(fmt.Sprintf("Configuration reload hook panicked: %v", r)) + } + }() + hook(cfg) +} diff --git a/config/reload_test.go b/config/reload_test.go new file mode 100644 index 0000000..1162c17 --- /dev/null +++ b/config/reload_test.go @@ -0,0 +1,138 @@ +package config + +import ( + "testing" + + "nukumizu-backend/global" +) + +// swapReloadHooks replaces the registered reload hooks for the duration of a +// test and restores the previous set afterwards, so the package-level registry +// cannot leak into another test. +func swapReloadHooks(t *testing.T, hooks ...func(*Config)) { + t.Helper() + + reloadHooksMu.Lock() + original := reloadHooks + reloadHooks = hooks + reloadHooksMu.Unlock() + + t.Cleanup(func() { + reloadHooksMu.Lock() + reloadHooks = original + reloadHooksMu.Unlock() + }) +} + +func TestReloadHookRunsOnlyForGlobalSettings(t *testing.T) { + writeTempConfig(t, &global.ConfigPath.Global, `{"system":{"debugMode":false}}`) + writeTempConfig(t, &global.ConfigPath.BotUserConfig, `{}`) + writeTempConfig(t, &global.ConfigPath.BotNodeConfig, `{}`) + + var seen []bool + swapReloadHooks(t, func(cfg *Config) { + seen = append(seen, cfg.System.DebugMode) + }) + + // A config.json update runs the hook, with the values just written. + if err := UpdateSettings(SettingGlobal, map[string]interface{}{ + "system": map[string]interface{}{"debugMode": true}, + }); err != nil { + t.Fatalf("UpdateSettings(global): %v", err) + } + + if len(seen) != 1 { + t.Fatalf("hook ran %d times for a config.json update, want 1", len(seen)) + } + if !seen[0] { + t.Error("hook received a configuration without the updated debugMode") + } + + // The other two files reload a singleton that callers read at the point of + // use, so they have nothing to notify. + for _, settingsType := range []string{SettingBotUserConfig, SettingBotNodeConfig} { + if err := UpdateSettings(settingsType, map[string]interface{}{ + "unused": map[string]interface{}{"event_reply": true}, + }); err != nil { + t.Fatalf("UpdateSettings(%s): %v", settingsType, err) + } + } + + if len(seen) != 1 { + t.Errorf("hook ran %d times after updates to the other settings files, want 1", len(seen)) + } +} + +func TestReloadHookIsolatesPanic(t *testing.T) { + writeTempConfig(t, &global.ConfigPath.Global, `{"system":{"debugMode":false}}`) + + reached := false + swapReloadHooks(t, + func(*Config) { panic("hook under test") }, + func(*Config) { reached = true }, + ) + + // The configuration is already on disk and published by the time hooks run, + // so a broken hook must not turn a successful write into a failed request. + if err := UpdateSettings(SettingGlobal, map[string]interface{}{ + "system": map[string]interface{}{"debugMode": true}, + }); err != nil { + t.Fatalf("a panicking hook must not fail the settings write: %v", err) + } + if !reached { + t.Error("a panicking hook stopped the hooks registered after it") + } + + // The write itself must still have landed. + cfg := Current() + if cfg == nil || !cfg.System.DebugMode { + t.Error("the settings write did not take effect") + } +} + +// TestReloadHookSeesEveryWriterPath pins the invariant that each writer of +// config.json notifies, not just /api/settings/set: the webhook endpoint +// helpers go through the same channel. +func TestReloadHookSeesEveryWriterPath(t *testing.T) { + writeTempConfig(t, &global.ConfigPath.Global, `{ + "webhook": { "endpoints": {} } +}`) + + var seen int + swapReloadHooks(t, func(*Config) { seen++ }) + + if err := AddWebhookEndpoint("example", map[string]interface{}{ + "enabled": true, + "token": "secret", + "notifyPipes": []interface{}{"ntfy"}, + }); err != nil { + t.Fatalf("AddWebhookEndpoint: %v", err) + } + if seen != 1 { + t.Errorf("hook ran %d times after adding an endpoint, want 1", seen) + } + + if err := ModifyWebhookEndpoint("example", map[string]interface{}{ + "enabled": false, + }); err != nil { + t.Fatalf("ModifyWebhookEndpoint: %v", err) + } + if seen != 2 { + t.Errorf("hook ran %d times after modifying an endpoint, want 2", seen) + } + + if err := DeleteWebhookEndpoint("example"); err != nil { + t.Fatalf("DeleteWebhookEndpoint: %v", err) + } + if seen != 3 { + t.Errorf("hook ran %d times after deleting an endpoint, want 3", seen) + } + + // A rejected write changes nothing, so it must not notify either. + if err := DeleteWebhookEndpoint("ghost"); err == nil { + t.Error("deleting an unknown endpoint should fail") + } + if seen != 3 { + t.Errorf("hook ran for a rejected write (%d notifications, want 3)", seen) + } +} diff --git a/config/settings.go b/config/settings.go index 5cc91e8..15a3868 100644 --- a/config/settings.go +++ b/config/settings.go @@ -82,19 +82,45 @@ func GetSettings(settingsType string) ([]byte, error) { // written the matching in-memory singleton is reloaded so runtime code observes // the new values. func UpdateSettings(settingsType string, patch map[string]interface{}) error { - settingsLock.Lock() - defer settingsLock.Unlock() + return runSettingsUpdate(func() (*Config, error) { + return updateSettingsLocked(settingsType, patch) + }) +} - return updateSettingsLocked(settingsType, patch) +// runSettingsUpdate runs fn under settingsLock and then, once the lock is +// released, runs the reload hooks with whatever configuration fn reports (nil +// when the update did not touch config.json). +// +// The hooks deliberately run outside settingsLock. A hook rebuilds controllers, +// which can wait on a network call, while settingsLock is also held by the node +// tracker's background save (SaveBotNodeConfig); holding it across a hook would +// stall node registration behind an unrelated settings edit. +func runSettingsUpdate(fn func() (*Config, error)) error { + cfg, err := func() (*Config, error) { + settingsLock.Lock() + defer settingsLock.Unlock() + return fn() + }() + if err != nil { + return err + } + + notifyReload(cfg) + return nil } // updateSettingsLocked is UpdateSettings without the locking, for callers that // need to inspect the loaded configuration and write in one critical section // (see the incoming webhook endpoint helpers). Callers must hold settingsLock. -func updateSettingsLocked(settingsType string, patch map[string]interface{}) error { +// +// It returns the freshly loaded global configuration, or nil when the settings +// type is one of the other files. The caller is responsible for handing that +// value to notifyReload once settingsLock is released — which runSettingsUpdate +// does for every writer. +func updateSettingsLocked(settingsType string, patch map[string]interface{}) (*Config, error) { path, err := settingsPath(settingsType) if err != nil { - return err + return nil, err } // Start from whatever is already on disk so nothing is dropped. A missing or @@ -104,23 +130,23 @@ func updateSettingsLocked(settingsType string, patch map[string]interface{}) err if err == nil { if len(bytes.TrimSpace(data)) > 0 { if err := json.Unmarshal(data, ¤t); err != nil { - return fmt.Errorf("failed to parse existing %s settings file %s: %w", settingsType, path, err) + return nil, fmt.Errorf("failed to parse existing %s settings file %s: %w", settingsType, path, err) } } } else if !os.IsNotExist(err) { - return fmt.Errorf("failed to read existing %s settings file %s: %w", settingsType, path, err) + return nil, fmt.Errorf("failed to read existing %s settings file %s: %w", settingsType, path, err) } deepMergeSettings(current, patch) data, err = json.MarshalIndent(current, "", " ") if err != nil { - return fmt.Errorf("failed to marshal %s settings: %w", settingsType, err) + return nil, fmt.Errorf("failed to marshal %s settings: %w", settingsType, err) } data = append(data, '\n') if err := os.WriteFile(path, data, 0o644); err != nil { - return fmt.Errorf("failed to write %s settings file %s: %w", settingsType, path, err) + return nil, fmt.Errorf("failed to write %s settings file %s: %w", settingsType, path, err) } return reloadSettings(settingsType, path) @@ -151,18 +177,19 @@ func deepMergeSettings(dst, src map[string]interface{}) { } // reloadSettings refreshes the in-memory singleton for the given settings type -// so the running program observes the values just persisted to disk. -func reloadSettings(settingsType, path string) error { +// so the running program observes the values just persisted to disk. Only +// config.json has a hook-visible reload, so for SettingGlobal it returns the +// configuration now in effect and for the other types it returns nil. +func reloadSettings(settingsType, path string) (*Config, error) { switch settingsType { case SettingGlobal: - _, err := LoadGlobalConfig(path) - return err + return LoadGlobalConfig(path) case SettingBotUserConfig: _, err := LoadBotUserConfig(path) - return err + return nil, err case SettingBotNodeConfig: - return LoadBotNodeConfig(path) + return nil, LoadBotNodeConfig(path) default: - return ErrUnsupportedSettingsType + return nil, ErrUnsupportedSettingsType } } diff --git a/config/webhook.go b/config/webhook.go index 11efbe8..f8e59d2 100644 --- a/config/webhook.go +++ b/config/webhook.go @@ -60,13 +60,12 @@ func AddWebhookEndpoint(name string, fields map[string]interface{}) error { return err } - settingsLock.Lock() - defer settingsLock.Unlock() - - if _, exists := webhookEndpoint(name); exists { - return fmt.Errorf("%w: %s", ErrWebhookEndpointExists, name) - } - return updateSettingsLocked(SettingGlobal, webhookEndpointsPatch(name, patch)) + return runSettingsUpdate(func() (*Config, error) { + if _, exists := webhookEndpoint(name); exists { + return nil, fmt.Errorf("%w: %s", ErrWebhookEndpointExists, name) + } + return updateSettingsLocked(SettingGlobal, webhookEndpointsPatch(name, patch)) + }) } // ModifyWebhookEndpoint updates an existing incoming webhook endpoint. Only the @@ -84,27 +83,24 @@ func ModifyWebhookEndpoint(name string, fields map[string]interface{}) error { return fmt.Errorf("%w: no fields to update", ErrWebhookEndpointInvalid) } - settingsLock.Lock() - defer settingsLock.Unlock() - - if _, exists := webhookEndpoint(name); !exists { - return fmt.Errorf("%w: %s", ErrWebhookEndpointNotFound, name) - } - return updateSettingsLocked(SettingGlobal, webhookEndpointsPatch(name, patch)) + return runSettingsUpdate(func() (*Config, error) { + if _, exists := webhookEndpoint(name); !exists { + return nil, fmt.Errorf("%w: %s", ErrWebhookEndpointNotFound, name) + } + return updateSettingsLocked(SettingGlobal, webhookEndpointsPatch(name, patch)) + }) } // DeleteWebhookEndpoint removes the incoming webhook endpoint registered under // name. The endpoint stops accepting requests as soon as the configuration is // reloaded. func DeleteWebhookEndpoint(name string) error { - settingsLock.Lock() - defer settingsLock.Unlock() - - if _, exists := webhookEndpoint(name); !exists { - return fmt.Errorf("%w: %s", ErrWebhookEndpointNotFound, name) - } - - return updateSettingsLocked(SettingGlobal, webhookEndpointDeletePatch(name)) + return runSettingsUpdate(func() (*Config, error) { + if _, exists := webhookEndpoint(name); !exists { + return nil, fmt.Errorf("%w: %s", ErrWebhookEndpointNotFound, name) + } + return updateSettingsLocked(SettingGlobal, webhookEndpointDeletePatch(name)) + }) } // webhookEndpoint returns the named endpoint held by the loaded configuration. diff --git a/main.go b/main.go index a1bc0ed..caddf82 100644 --- a/main.go +++ b/main.go @@ -58,6 +58,14 @@ func main() { postLog.SetDebugMode(cfg.System.DebugMode) postLog.InitLogBroadcaster() + // The logger's debug flag is a process-wide setting rather than something + // read at every log call, so a settings update has to push the new value + // into it. This is the first link in the reload hook chain; the controllers + // join it as they gain reload support. + config.OnReload(func(updated *config.Config) { + postLog.SetDebugMode(updated.System.DebugMode) + }) + dbPath := cfg.DBPath if err := postLog.InitLogsDatabase(fmt.Sprintf("%s/log.db", dbPath)); err != nil {