From 03e74f49c36ba05995651135bd0f472b95bfede6 Mon Sep 17 00:00:00 2001 From: NanamiAdmin Date: Mon, 12 Oct 2026 17:17:55 +0800 Subject: [PATCH] feat(controller): add flap inhibition for each controllers --- config/config.go | 29 +++- config/flap_test.go | 63 +++++++ config/variables.go | 13 +- internal/controller/controller.go | 154 ++++++++++++++++- internal/controller/controller_test.go | 224 +++++++++++++++++++++++++ 5 files changed, 478 insertions(+), 5 deletions(-) create mode 100644 config/flap_test.go diff --git a/config/config.go b/config/config.go index a3ede6f..46f3027 100644 --- a/config/config.go +++ b/config/config.go @@ -5,13 +5,14 @@ import ( "encoding/json" "fmt" "os" + "time" ) // LoadBotNodeConfig reads and parses bot_node_config.json and stores it as the // global C_botNodeConfig singleton, mirroring LoadGlobalConfig. Unlike the // other config files this one is auto-generated and optional: a missing or -// empty file yields an empty map, so every node falls back to its default -// enableStatusNotify value (true). +// empty file yields an empty map, so every node falls back to its defaults — +// enableStatusNotify true and flap_tolerant 0 (no flap inhibition). func LoadBotNodeConfig(configPath string) error { cfg := BotNodeMembers{} data, err := os.ReadFile(configPath) @@ -44,6 +45,27 @@ func NodeStatusNotifyEnabled(uuid string) bool { return *opts.EnableStatusNotify } +// maxFlapTolerantSeconds bounds the flap-inhibition window so an absurd +// hand-written value cannot overflow time.Duration. +const maxFlapTolerantSeconds = 86400 + +// NodeFlapTolerant returns how long an Offline notification for uuid should be +// held in order to absorb a quick reconnect, per bot_node_config.json's +// flap_tolerant. It returns 0 (inhibition disabled) when the node has no entry, +// the value is zero or negative, or the config has not been loaded — in every +// case the caller falls back to notifying immediately. +func NodeFlapTolerant(uuid string) time.Duration { + opts, ok := BotNodes()[uuid] + if !ok || opts.FlapTolerant <= 0 { + return 0 + } + seconds := opts.FlapTolerant + if seconds > maxFlapTolerantSeconds { + seconds = maxFlapTolerantSeconds + } + return time.Duration(seconds * float64(time.Second)) +} + // LoadGlobalConfig reads and parses the configuration file, applies defaults, // and stores it as a global singleton. func LoadGlobalConfig(configPath string) (*Config, error) { @@ -168,7 +190,8 @@ func LoadBotUserConfig(configPath string) (*BotUserConfig, error) { // which is a plain registry of the nodes Komari reports (read back at startup // by LoadBotNodeConfig). Entries already present are always preserved: a UUID // Komari no longer reports on a given fetch is kept rather than deleted, and -// any enableStatusNotify a user set by hand is left untouched. UUIDs seen for +// any per-node option a user set by hand — enableStatusNotify or flap_tolerant +// — is left untouched. UUIDs seen for // the first time are added as an empty object, i.e. no parameter is written, so // they inherit the enableStatusNotify default (true). The resulting JSON has // its object keys emitted in sorted order by encoding/json, keeping the file diff --git a/config/flap_test.go b/config/flap_test.go new file mode 100644 index 0000000..9b8d106 --- /dev/null +++ b/config/flap_test.go @@ -0,0 +1,63 @@ +package config + +import ( + "testing" + "time" + + "nukumizu-backend/global" +) + +// TestNodeFlapTolerant covers the flap-inhibition window accessor against the +// per-node options in bot_node_config.json, including the hand-written values +// the comment on BotNodeOptions.FlapTolerant promises to accept. +func TestNodeFlapTolerant(t *testing.T) { + writeTempConfig(t, &global.ConfigPath.BotNodeConfig, `{ + "seconds": { "flap_tolerant": 30 }, + "fractional": { "flap_tolerant": 2.5 }, + "zero": { "flap_tolerant": 0 }, + "negative": { "flap_tolerant": -5 }, + "unset": { "enableStatusNotify": true }, + "absurd": { "flap_tolerant": 1e9 }, + "throughput": { "flap_tolerant": 8, "enableStatusNotify": false } +}`) + if err := LoadBotNodeConfig(global.ConfigPath.BotNodeConfig); err != nil { + t.Fatalf("LoadBotNodeConfig: %v", err) + } + + cases := []struct { + name string + uuid string + want time.Duration + }{ + {"whole seconds", "seconds", 30 * time.Second}, + {"fractional seconds parse", "fractional", 2500 * time.Millisecond}, + {"zero disables", "zero", 0}, + {"negative disables", "negative", 0}, + {"absent field disables", "unset", 0}, + {"unknown node disables", "not-in-the-file", 0}, + {"absurd value is clamped", "absurd", maxFlapTolerantSeconds * time.Second}, + {"independent of enableStatusNotify", "throughput", 8 * time.Second}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := NodeFlapTolerant(tc.uuid); got != tc.want { + t.Errorf("NodeFlapTolerant(%q) = %v, want %v", tc.uuid, got, tc.want) + } + }) + } +} + +// TestNodeFlapTolerantWithoutConfig pins the nil-safe default: before any +// bot_node_config.json has been loaded — and for a file that is missing or +// empty — no node has a window, so notifications keep their previous +// notify-immediately behavior. +func TestNodeFlapTolerantWithoutConfig(t *testing.T) { + writeTempConfig(t, &global.ConfigPath.BotNodeConfig, ``) + if err := LoadBotNodeConfig(global.ConfigPath.BotNodeConfig); err != nil { + t.Fatalf("LoadBotNodeConfig: %v", err) + } + + if got := NodeFlapTolerant("anything"); got != 0 { + t.Errorf("node flap tolerance from an empty file = %v, want 0", got) + } +} diff --git a/config/variables.go b/config/variables.go index 29277d9..32aa6e0 100644 --- a/config/variables.go +++ b/config/variables.go @@ -257,13 +257,24 @@ type BotNodeOptions struct { // bool) lets an absent field be told apart from an explicit false, and // omitempty keeps untouched nodes stored as {}. EnableStatusNotify *bool `json:"enableStatusNotify,omitempty"` + + // FlapTolerant is the number of seconds a node's Offline notification is + // held in the hope the node returns. A node that comes back online within + // this window produces no notification at all; one still offline when the + // window elapses reports Offline as usual. Zero (or an absent field) + // disables inhibition, which keeps the notify-immediately behavior. It is + // a float64 rather than an int so a hand-written fractional value such as + // 2.5 parses instead of failing the whole config load at startup, and it + // is not a pointer because 0 already means "off", unlike + // EnableStatusNotify whose default is true. + FlapTolerant float64 `json:"flap_tolerant,omitempty"` } // BotNodeMembers maps a node UUID (as reported by Komari) to its per-node // options. type BotNodeMembers map[string]BotNodeOptions -// botNodeConfig mirrors bot_node_config.json, populated by LoadBotNodeConfig. +// NodeConfig mirrors bot_node_config.json, populated by LoadNodeConfig. // 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] diff --git a/internal/controller/controller.go b/internal/controller/controller.go index 7144533..a8a1a9f 100644 --- a/internal/controller/controller.go +++ b/internal/controller/controller.go @@ -7,6 +7,7 @@ import ( "strings" "sync" "sync/atomic" + "time" "nukumizu-backend/config" "nukumizu-backend/internal/node" @@ -107,6 +108,15 @@ type BotController interface { SendMessage(message Message) error } +// flapPending is an Offline notification being held back for a node's +// flap_tolerant window, in the hope that the node returns before the deadline. +// The held change is kept by value so a later mutation of the caller's copy +// cannot reach it. +type flapPending struct { + timer *time.Timer + change node.StatusChange +} + // Manager manages all controller instances and routes events. type Manager struct { mu sync.RWMutex @@ -117,6 +127,19 @@ type Manager struct { // from one that does not. It is read on the settings-update goroutine and // written when the set is replaced. builtFrom atomic.Pointer[config.ControllerMethodConfig] + + // flapMu guards flapPending, the Offline notifications currently held for + // flap inhibition, keyed by node UUID. It is a separate lock from mu and is + // never held while mu is taken: a held notification is dispatched by a timer + // goroutine, which must not block behind a routing swap, and dispatching + // (network I/O per controller) must not happen while holding the flap lock. + flapMu sync.Mutex + flapPending map[string]*flapPending + + // flapToleranceFor overrides the per-node flap window. It is nil in + // production, where the value comes from bot_node_config.json through + // config.NodeFlapTolerant; tests set it to keep their timings hermetic. + flapToleranceFor func(uuid string) time.Duration } var globalManager *Manager @@ -125,6 +148,7 @@ var globalManager *Manager func InitManager() { globalManager = &Manager{ controllers: make(map[string]Controller), + flapPending: make(map[string]*flapPending), } postLog.Info("Controller manager initialized") } @@ -268,12 +292,37 @@ func (m *Manager) ShowBotServerList() { // controllers. It honors the per-node allow-list in bot_node_config.json: a // node whose enableStatusNotify is not true is skipped entirely, so no // controller (chat bots or notification pipes) broadcasts its change. +// +// It also applies flap inhibition, the counterpart of the allow-list and read +// from the same file: an Offline notification for a node with a positive +// flap_tolerant is held back for that many seconds instead of being broadcast +// (see holdOffline). Because both gates live here, every controller is subject +// to them, chat bots and notification pipes alike. func (m *Manager) NotifyStatusChange(change node.StatusChange) { if !config.NodeStatusNotifyEnabled(change.UUID) { postLog.Debug(fmt.Sprintf("Status change for node %s skipped: enableStatusNotify is not enabled", change.UUID)) return } + switch change.Event { + case "Offline": + if tolerance := m.flapTolerance(change.UUID); tolerance > 0 { + m.holdOffline(change, tolerance) + return + } + case "Online": + // A node that returns while its Offline is still held flaps: both + // halves of the pair are suppressed, and nothing is sent at all. + if m.releaseHeldOffline(change.UUID) { + return + } + } + + m.dispatchStatusChange(change) +} + +// dispatchStatusChange hands a change to every enabled controller. +func (m *Manager) dispatchStatusChange(change node.StatusChange) { m.mu.RLock() defer m.mu.RUnlock() @@ -287,6 +336,98 @@ func (m *Manager) NotifyStatusChange(change node.StatusChange) { } } +// flapTolerance returns the flap-inhibition window for a node, honoring the +// test override when one is set. +func (m *Manager) flapTolerance(uuid string) time.Duration { + if m.flapToleranceFor != nil { + return m.flapToleranceFor(uuid) + } + return config.NodeFlapTolerant(uuid) +} + +// holdOffline holds an Offline notification for the given window rather than +// broadcasting it. If the node returns within the window, releaseHeldOffline +// claims the held notification first and nothing is sent; otherwise the timer +// claims it and the Offline goes out late (see fireHeldOffline). +// +// A held notification is claimed by removing it from flapPending under flapMu, +// and the claim is decided by the identity of the map entry. That is what keeps +// the two racers — the timer and a returning Online — from both winning: +// whichever gets there first deletes the entry, and the loser finds it gone. +func (m *Manager) holdOffline(change node.StatusChange, tolerance time.Duration) { + m.flapMu.Lock() + if m.flapPending == nil { + m.flapPending = make(map[string]*flapPending) + } + // Defensive: the tracker reports transitions, so a second Offline should + // not arrive for a node already held, but if one does the older hold is + // superseded — its callback loses the claim below and drops the change. + if old := m.flapPending[change.UUID]; old != nil { + old.timer.Stop() + } + held := &flapPending{change: change} + held.timer = time.AfterFunc(tolerance, func() { + m.fireHeldOffline(change.UUID, held) + }) + m.flapPending[change.UUID] = held + m.flapMu.Unlock() + + postLog.Debug(fmt.Sprintf("Offline for node %s held for %s (flap tolerance)", change.UUID, tolerance)) +} + +// fireHeldOffline dispatches a held Offline once its window has elapsed. It +// claims the entry first and returns without sending if the hold was already +// released or superseded, so a node that came back in time never reports an +// Offline. +func (m *Manager) fireHeldOffline(uuid string, held *flapPending) { + m.flapMu.Lock() + if m.flapPending[uuid] != held { + // Already claimed by a returning Online, or replaced by a newer hold. + m.flapMu.Unlock() + return + } + delete(m.flapPending, uuid) + m.flapMu.Unlock() + + // Re-check the allow-list: it may have been edited while the change was + // held, and years-long holds are possible in principle. + if !config.NodeStatusNotifyEnabled(uuid) { + postLog.Debug(fmt.Sprintf("Held status change for node %s dropped: enableStatusNotify is not enabled", uuid)) + return + } + + postLog.Info(fmt.Sprintf("Node %s still offline after flap tolerance; reporting it now", uuid)) + m.dispatchStatusChange(held.change) +} + +// releaseHeldOffline claims and cancels a held Offline notification, reporting +// whether one was still pending. A true result means the node returned within +// its flap window, so the caller must suppress the Online as well. +// +// Stop's return value is deliberately ignored: false also covers "the timer has +// started firing but has not yet taken flapMu", which is not the same as +// "already dispatched". Removing the entry is what decides the race, and it +// makes the only possible outcomes "both halves sent" (the timer won, the node +// really did stay down past the deadline) or "neither sent" — never an Offline +// with no matching Online. +func (m *Manager) releaseHeldOffline(uuid string) bool { + m.flapMu.Lock() + defer m.flapMu.Unlock() + + if m.flapPending == nil { + return false + } + held := m.flapPending[uuid] + if held == nil { + return false + } + held.timer.Stop() + delete(m.flapPending, uuid) + + postLog.Info(fmt.Sprintf("Node %s returned within flap tolerance; suppressed both notifications", uuid)) + return true +} + // NotifyAlert delivers an alert to the named pipes only, and returns the names // of the pipes it was handed to. A pipe that is unknown, disabled or fails to // send is reported through the returned error instead of stopping the delivery @@ -340,8 +481,19 @@ func (m *Manager) IsMarkdown(pipeName string) bool { return ctrl.IsMarkdown() } -// StopAll stops all registered controllers. +// StopAll stops all registered controllers. Held Offline notifications are +// dropped first: their timers would otherwise fire against a shutting-down +// controller set. A timer that has already claimed its entry can still be +// dispatching concurrently, which is harmless — it reads the same registry +// under the same lock. func (m *Manager) StopAll() { + m.flapMu.Lock() + for uuid, held := range m.flapPending { + held.timer.Stop() + delete(m.flapPending, uuid) + } + m.flapMu.Unlock() + m.mu.RLock() defer m.mu.RUnlock() for _, ctrl := range m.controllers { diff --git a/internal/controller/controller_test.go b/internal/controller/controller_test.go index a500d69..e53f4c1 100644 --- a/internal/controller/controller_test.go +++ b/internal/controller/controller_test.go @@ -1,6 +1,8 @@ package controller import ( + "os" + "path/filepath" "sync" "testing" "time" @@ -79,6 +81,228 @@ func (m *Manager) routedTo(name string) Controller { return m.controllers[name] } +// heldOffline returns the Offline notification the manager is currently holding +// for the given node, if any. It lets a test drive the hold/fire/release race +// deterministically instead of racing a real timer. +func (m *Manager) heldOffline(uuid string) *flapPending { + m.flapMu.Lock() + defer m.flapMu.Unlock() + return m.flapPending[uuid] +} + +// recordingController keeps every status change it is handed, so a test can +// assert on what actually reached the controllers. The embedded fakeController +// satisfies the rest of the Controller interface. +type recordingController struct { + *fakeController + changes chan node.StatusChange +} + +func newRecording(name string) *recordingController { + return &recordingController{ + fakeController: newFake(name), + changes: make(chan node.StatusChange, 16), + } +} + +func (r *recordingController) SendStatusChange(change node.StatusChange) error { + r.changes <- change + return nil +} + +// awaitChange blocks until a status change reaches the controller, failing the +// test rather than hanging if none does. +func awaitChange(t *testing.T, rec *recordingController, what string) node.StatusChange { + t.Helper() + select { + case change := <-rec.changes: + return change + case <-time.After(5 * time.Second): + t.Fatalf("timed out waiting for the %s notification", what) + return node.StatusChange{} + } +} + +// expectNoChange asserts that no status change reaches the controller for the +// given window. Only a negative assertion needs a real wait; the manager has no +// observable "decided to stay silent" event. +func expectNoChange(t *testing.T, rec *recordingController, window time.Duration, what string) { + t.Helper() + select { + case change := <-rec.changes: + t.Fatalf("unexpected %s notification: %+v", what, change) + case <-time.After(window): + } +} + +func offlineChange(uuid string) node.StatusChange { + return node.StatusChange{Event: "Offline", UUID: uuid, Name: uuid, OldOnline: true, NewOnline: false} +} + +func onlineChange(uuid string) node.StatusChange { + return node.StatusChange{Event: "Online", UUID: uuid, Name: uuid, OldOnline: false, NewOnline: true} +} + +// flapTestManager returns a manager with a recording controller installed and +// the flap window stubbed to tolerance, so the tests never touch the global +// bot_node_config.json or wait on real seconds. ReplaceAll makes the controller +// routable before it starts it, so no start handshake is needed here. +func flapTestManager(tolerance time.Duration) (*Manager, *recordingController) { + m := newTestManager() + m.flapToleranceFor = func(string) time.Duration { return tolerance } + rec := newRecording("recorder") + m.ReplaceAll([]Controller{rec}, config.ControllerMethodConfig{}) + return m, rec +} + +// TestFlapInhibitionSuppressesBothHalves is the core guarantee: a node that +// returns inside its flap window produces no notification at all, so a flapping +// node does not flood the controllers. +func TestFlapInhibitionSuppressesBothHalves(t *testing.T) { + m, rec := flapTestManager(50 * time.Millisecond) + + m.NotifyStatusChange(offlineChange("flappy")) + m.NotifyStatusChange(onlineChange("flappy")) + + expectNoChange(t, rec, 300*time.Millisecond, "suppressed offline/online pair") + + if held := m.heldOffline("flappy"); held != nil { + t.Error("the held offline notification was not released when the node returned") + } +} + +// TestFlapInhibitionReportsLateOffline: a node still down when the window +// elapses reports Offline as usual, however late the notification is. +func TestFlapInhibitionReportsLateOffline(t *testing.T) { + m, rec := flapTestManager(50 * time.Millisecond) + + m.NotifyStatusChange(offlineChange("down")) + + if change := awaitChange(t, rec, "offline"); change.Event != "Offline" || change.UUID != "down" { + t.Errorf("got %+v, want the Offline change for node down", change) + } + if held := m.heldOffline("down"); held != nil { + t.Error("the held notification was still pending after it was dispatched") + } +} + +// TestFlapInhibitionReportsRecoveryAfterWindow: once the Offline has gone out, +// the node's eventual return is a genuine recovery and must be reported. This +// is the honest half of the "both or neither, never Offline alone" rule. +func TestFlapInhibitionReportsRecoveryAfterWindow(t *testing.T) { + m, rec := flapTestManager(50 * time.Millisecond) + + m.NotifyStatusChange(offlineChange("down")) + if change := awaitChange(t, rec, "offline"); change.Event != "Offline" { + t.Fatalf("got %+v, want the Offline change", change) + } + + m.NotifyStatusChange(onlineChange("down")) + if change := awaitChange(t, rec, "online"); change.Event != "Online" { + t.Errorf("got %+v, want the Online change", change) + } +} + +// TestFlapInhibitionDisabledWithoutTolerance guards the previous behavior: with +// no window (the default for every node), an Offline still goes out at once. +func TestFlapInhibitionDisabledWithoutTolerance(t *testing.T) { + m, rec := flapTestManager(0) + + m.NotifyStatusChange(offlineChange("plain")) + + if change := awaitChange(t, rec, "offline"); change.Event != "Offline" { + t.Errorf("got %+v, want the Offline change", change) + } + if held := m.heldOffline("plain"); held != nil { + t.Error("a node without a flap window must not have a held notification") + } +} + +// TestFlapInhibitionHonorsEnableStatusNotify pins the gate ordering: a node +// whose notifications are switched off is skipped before any hold is taken, so +// it cannot report a late Offline either. +func TestFlapInhibitionHonorsEnableStatusNotify(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "bot_node_config.json") + content := `{"muted": {"enableStatusNotify": false, "flap_tolerant": 600}}` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("write node config: %v", err) + } + if err := config.LoadBotNodeConfig(path); err != nil { + t.Fatalf("LoadBotNodeConfig: %v", err) + } + // Leave nothing behind for the other tests in this package, which reach the + // same package-global node config. + t.Cleanup(func() { + if err := config.LoadBotNodeConfig(filepath.Join(dir, "empty.json")); err != nil { + t.Errorf("reset node config: %v", err) + } + }) + + // The stub reports a window, so only the allow-list can be responsible for + // the silence. + m, rec := flapTestManager(time.Hour) + m.NotifyStatusChange(offlineChange("muted")) + + expectNoChange(t, rec, 100*time.Millisecond, "offline for a muted node") + if held := m.heldOffline("muted"); held != nil { + t.Error("a muted node must not have its Offline held") + } +} + +// TestFlapInhibitionClaimOrder pins both sides of the hold/fire/release race +// deterministically, with a window long enough that the real timer never fires: +// whichever claims the held notification first wins, and the loser sees it gone. +func TestFlapInhibitionClaimOrder(t *testing.T) { + t.Run("release first suppresses the pair", func(t *testing.T) { + m, rec := flapTestManager(time.Hour) + m.NotifyStatusChange(offlineChange("node")) + + held := m.heldOffline("node") + if held == nil { + t.Fatal("the offline notification was not held") + } + if !m.releaseHeldOffline("node") { + t.Fatal("releaseHeldOffline reported no pending notification") + } + // The timer goroutine loses the claim and must not dispatch. + m.fireHeldOffline("node", held) + + expectNoChange(t, rec, 100*time.Millisecond, "notification after a release") + }) + + t.Run("fire first reports the offline alone", func(t *testing.T) { + m, rec := flapTestManager(time.Hour) + m.NotifyStatusChange(offlineChange("node")) + + held := m.heldOffline("node") + if held == nil { + t.Fatal("the offline notification was not held") + } + m.fireHeldOffline("node", held) + + if change := awaitChange(t, rec, "offline"); change.Event != "Offline" { + t.Errorf("got %+v, want the Offline change", change) + } + if m.releaseHeldOffline("node") { + t.Error("a fired notification must not be claimable again") + } + }) +} + +// TestStopAllDropsHeldNotifications: a hold must not outlive the controllers it +// was meant for. +func TestStopAllDropsHeldNotifications(t *testing.T) { + m, _ := flapTestManager(time.Hour) + m.NotifyStatusChange(offlineChange("node")) + + m.StopAll() + + if held := m.heldOffline("node"); held != nil { + t.Error("StopAll left a held offline notification armed") + } +} + func TestReplaceAllInstallsAndStarts(t *testing.T) { m := newTestManager() alpha, beta := newFake("alpha"), newFake("beta")