From fde59d933ca5d9381716e7d25fba51b81f9c7b65 Mon Sep 17 00:00:00 2001 From: Ian Oberst Date: Thu, 22 Oct 2026 13:12:04 -0700 Subject: [PATCH 1/3] Fix stop-loss count and percent check Loader.Load used the number of removed monitors as the number left, and compared a 0-1 loss fraction against the whole-number percent from blip.StopLoss, so a percent stop-loss never triggered. Count the net drop (before - removed + new; changed monitors don't count) and compare lost*100 > limit*before for percent stop-loss. TestLoaderUnloadedMonitorsHaveNoCounters relied on the miscount; it now removes 3 of 4 monitors to get ErrStopLoss. Co-Authored-By: Claude Opus 5.5 --- docs/content/about/release-notes.md | 1 + docs/content/config/config-file.md | 4 +- docs/content/monitors/loading.md | 6 ++ monitor/loader.go | 32 +++++--- monitor/loader_counters_test.go | 6 +- monitor/loader_stoploss_test.go | 122 ++++++++++++++++++++++++++++ 6 files changed, 156 insertions(+), 15 deletions(-) create mode 100644 monitor/loader_stoploss_test.go diff --git a/docs/content/about/release-notes.md b/docs/content/about/release-notes.md index 10fca47..4367805 100644 --- a/docs/content/about/release-notes.md +++ b/docs/content/about/release-notes.md @@ -65,6 +65,7 @@ Default sink HTTP clients now have a 10-second whole-request timeout plus bounde * **Behavior change:** The `repl.lag` heartbeat reader now uses `repl-check`; previously it ignored it at every level. With the Blip heartbeat writer and `repl-check` set, an instance that isn't a replica now drops `repl.lag.current`, or reports -1 with `report-not-a-replica=yes`, instead of reporting heartbeat lag. * **Behavior change:** Plans that set the `repl.lag` `writer` to `pfs` at one level and `blip` at another now fail to prepare, and the monitor keeps retrying the plan. Previously each level used its own writer. Set the same `writer` at every level, or set it at one level only. * Changed `heartbeat.BlipReader.Stop` to wait for the reader to return, which takes up to the heartbeat read timeout (2s) if a heartbeat query is running. Previously the reader could report status after its monitor was stopped and removed, so the removed monitor reappeared in [`GET /status/monitors`]({{< ref "/api/status#get-statusmonitors" >}}). +* **Behavior change:** Fixed `monitor-loader.stop-loss` to count the net drop in the number of monitors. Previously it compared the number of removed monitors with the number loaded, so a small removal could trigger stop-loss and removing every monitor could not. A percent stop-loss (for example, `50%`) never triggered; it now triggers when the drop is greater than that percent of the monitors loaded before the reload. ### v2.1.0 (10 Aug 2026) diff --git a/docs/content/config/config-file.md b/docs/content/config/config-file.md index c96376b..3acf17e 100644 --- a/docs/content/config/config-file.md +++ b/docs/content/config/config-file.md @@ -178,7 +178,9 @@ The `local` subsection has only two variables: |**Valid values**|• "N%" (percentage) where N is an integer between 0 and 100 (exclusive)
• "N" where N is an integer greater than 0| |**Default value**|| -The `stop-loss` variable enables the [stop-lost feature]({{< ref "/monitors/loading#stop-loss" >}}). +The `stop-loss` variable enables the [stop-loss feature]({{< ref "/monitors/loading#stop-loss" >}}). +A reload fails if the net drop in the number of monitors is greater than `N` monitors, or greater than `N` percent of the monitors loaded before the reload. +For example, with 10 monitors loaded and `stop-loss: 30%`, a reload can drop 3 monitors but not 4. ##### `stop-timeout` diff --git a/docs/content/monitors/loading.md b/docs/content/monitors/loading.md index 5a72945..85a6b73 100644 --- a/docs/content/monitors/loading.md +++ b/docs/content/monitors/loading.md @@ -54,4 +54,10 @@ Stop-loss prevents reloading from dropping too many MySQL instances due to unrel For example, maybe the `LoadMonitors` plugin has a bug that causes it to return zero monitors when there are really 10 monitors. Stop-loss would prevent Blip from unloading all monitors, which could have adverse side effects for visibility and alerting. +Stop-loss is the net drop in the number of monitors: the number loaded before the reload minus the number that would be loaded after it. +New monitors offset removed monitors, and changed monitors don't count. +For example, with 10 monitors loaded, a reload that removes 4 monitors and adds 1 drops 3 monitors. +If the drop is greater than the stop-loss limit, the reload fails and the loaded monitors are not changed. +A drop equal to the limit is allowed. + Set [`config.monitor-loader.stop-loss`]({{< ref "/config/config-file#stop-loss" >}}) to enable. diff --git a/monitor/loader.go b/monitor/loader.go index c4f911b..24b53c5 100644 --- a/monitor/loader.go +++ b/monitor/loader.go @@ -311,20 +311,30 @@ func (ml *Loader) loadAndUnload(ctx context.Context) ([]*stoppingMonitor, error) // Check config.monitor-loader.stop-loss: don't change monitors if there's // a big drop in the number because it might be a false-positive that will // set off alarms when a bunch of metrics fail to report. - nBefore := float64(len(ml.repo)) - nNow := float64(len(diff.removed)) + // + // The loss is the net drop in the number of monitors: before is the number + // loaded now, and now is the number that would be loaded after this change. + // New monitors offset removed ones; changed monitors are in both diff.added + // and diff.changed, so they don't change the number. Stop-loss is a number + // or a percent, not both (see blip.StopLoss), and the loss must be greater + // than the limit to trigger it. + nBefore := len(ml.repo) + nNow := nBefore - len(diff.removed) + len(diff.added) - len(diff.changed) if nNow < nBefore { + lost := nBefore - nNow var errMsg string - if ml.stopLossPercent > 0 { - lost := (nBefore - nNow) / nBefore - if lost > ml.stopLossPercent { - errMsg = fmt.Sprintf("before: %d; now: %d; lost %f%% > limit %f%%", int(nBefore), int(nNow), lost, ml.stopLossPercent) + switch { + case ml.stopLossNumber > 0: + if uint(lost) > ml.stopLossNumber { + errMsg = fmt.Sprintf("before: %d; now: %d; lost %d > limit %d", nBefore, nNow, lost, ml.stopLossNumber) } - } - if ml.stopLossNumber > 0 { - lost := uint(nBefore - nNow) - if lost > ml.stopLossNumber { - errMsg = fmt.Sprintf("before: %d; now: %d; lost %d > limit %d", int(nBefore), int(nNow), lost, ml.stopLossNumber) + case ml.stopLossPercent > 0: + // stopLossPercent is a whole number (50 for 50%). Compare + // lost*100 > limit*before, not lost/before*100 > limit, so a + // loss exactly at the limit (3 of 10 for 30%) doesn't round + // over it. + if float64(lost*100) > ml.stopLossPercent*float64(nBefore) { + errMsg = fmt.Sprintf("before: %d; now: %d; lost %.1f%% > limit %.0f%%", nBefore, nNow, float64(lost*100)/float64(nBefore), ml.stopLossPercent) } } if errMsg != "" { diff --git a/monitor/loader_counters_test.go b/monitor/loader_counters_test.go index 1c9e546..7045878 100644 --- a/monitor/loader_counters_test.go +++ b/monitor/loader_counters_test.go @@ -154,10 +154,10 @@ func TestLoaderUnloadedMonitorsHaveNoCounters(t *testing.T) { t.Errorf("counters reported for loaded but not started monitors: %v", got) } - // Remove 2 of 4 monitors (stop-loss 1) and add a new one: Load makes the - // new monitor, then returns ErrStopLoss without adding it. + // Remove 3 of 4 monitors and add a new one, a net loss of 2 (stop-loss 1): + // Load makes the new monitor, then returns ErrStopLoss without adding it. mux.Lock() - configs = []blip.ConfigMonitor{configs[0], configs[1], chronosphereMonitor(newId, "1")} + configs = []blip.ConfigMonitor{configs[0], chronosphereMonitor(newId, "1")} mux.Unlock() if err := ml.Load(context.Background()); err != ErrStopLoss { t.Fatalf("Load returned %v, expected ErrStopLoss", err) diff --git a/monitor/loader_stoploss_test.go b/monitor/loader_stoploss_test.go new file mode 100644 index 0000000..98f0c42 --- /dev/null +++ b/monitor/loader_stoploss_test.go @@ -0,0 +1,122 @@ +// Copyright 2026 Block, Inc. + +package monitor_test + +import ( + "context" + "fmt" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/cashapp/blip/v2" + "github.com/cashapp/blip/v2/aws" + "github.com/cashapp/blip/v2/dbconn" + "github.com/cashapp/blip/v2/monitor" + "github.com/cashapp/blip/v2/plan" + "github.com/cashapp/blip/v2/test/mock" +) + +func TestLoaderStopLoss(t *testing.T) { + // Every case loads 10 monitors, then reloads after removing, adding, and + // changing some. Stop-loss is the net drop in the number of monitors: + // removed minus added. Changed monitors don't count. The drop must be + // greater than the limit to trigger stop-loss. + const nBefore = 10 + tests := []struct { + name string + stopLoss string + removed int + added int + changed int + stopped bool // expect ErrStopLoss + }{ + {name: "number below", stopLoss: "3", removed: 2}, + {name: "number at", stopLoss: "3", removed: 3}, + {name: "number above", stopLoss: "3", removed: 4, stopped: true}, + {name: "number all", stopLoss: "3", removed: 10, stopped: true}, + {name: "number below with new", stopLoss: "3", removed: 3, added: 1}, + {name: "number at with new", stopLoss: "3", removed: 4, added: 1}, + {name: "number above with new", stopLoss: "3", removed: 5, added: 1, stopped: true}, + {name: "number replaced", stopLoss: "3", removed: 5, added: 5}, + {name: "number changed", stopLoss: "3", changed: 5}, + {name: "percent below", stopLoss: "30%", removed: 2}, + {name: "percent at", stopLoss: "30%", removed: 3}, + {name: "percent above", stopLoss: "30%", removed: 4, stopped: true}, + {name: "percent all", stopLoss: "30%", removed: 10, stopped: true}, + {name: "percent below with new", stopLoss: "30%", removed: 3, added: 1}, + {name: "percent at with new", stopLoss: "30%", removed: 4, added: 1}, + {name: "percent above with new", stopLoss: "30%", removed: 5, added: 1, stopped: true}, + {name: "percent replaced", stopLoss: "30%", removed: 5, added: 5}, + {name: "percent changed", stopLoss: "30%", changed: 5}, + {name: "disabled", stopLoss: "", removed: 10}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var mux sync.Mutex + configs := make([]blip.ConfigMonitor, nBefore) + for i := range configs { + configs[i] = stopLossMonitor(fmt.Sprintf("stoploss-%d", i)) + } + ml := stopLossLoader(tt.stopLoss, func() []blip.ConfigMonitor { + mux.Lock() + defer mux.Unlock() + return configs + }) + require.NoError(t, ml.Load(context.Background())) + require.Equal(t, uint(nBefore), ml.Count()) + defer func() { + for _, mon := range ml.Monitors() { + ml.Unload(mon.MonitorId(), true) + } + }() + + // Remove the first monitors, change the next ones, and add new ones. + mux.Lock() + next := append([]blip.ConfigMonitor{}, configs[tt.removed:]...) + for i := 0; i < tt.changed; i++ { + next[i].Password = "changed" + } + for i := 0; i < tt.added; i++ { + next = append(next, stopLossMonitor(fmt.Sprintf("stoploss-new-%d", i))) + } + configs = next + mux.Unlock() + + err := ml.Load(context.Background()) + if tt.stopped { + assert.ErrorIs(t, err, monitor.ErrStopLoss) + assert.Equal(t, uint(nBefore), ml.Count(), "monitors changed after ErrStopLoss") + } else { + assert.NoError(t, err) + assert.Equal(t, uint(nBefore-tt.removed+tt.added), ml.Count()) + } + }) + } +} + +// stopLossLoader returns a Loader with the given stop-loss that loads the +// monitor configs returned by the configs func. The monitors aren't started. +func stopLossLoader(stopLoss string, configs func() []blip.ConfigMonitor) *monitor.Loader { + cfg := blip.DefaultConfig() + cfg.MonitorLoader.StopLoss = stopLoss + return monitor.NewLoader(monitor.LoaderArgs{ + Config: cfg, + Factories: blip.Factories{DbConn: dbconn.NewConnFactory(nil, nil)}, + PlanLoader: plan.NewLoader(nil), + RDSLoader: aws.RDSLoader{ClientFactory: mock.RDSClientFactory{}}, + Plugins: blip.Plugins{ + LoadMonitors: func(blip.Config) ([]blip.ConfigMonitor, error) { return configs(), nil }, + }, + }) +} + +func stopLossMonitor(monitorId string) blip.ConfigMonitor { + return blip.ConfigMonitor{ + MonitorId: monitorId, + Hostname: "127.0.0.1:1", + Sinks: map[string]map[string]string{"noop": {}}, + } +} From bb8d59299353c1e86541c7023ad423b5361de9ce Mon Sep 17 00:00:00 2001 From: Ian Oberst Date: Thu, 29 Oct 2026 07:01:20 -0700 Subject: [PATCH 2/3] Test stop-loss with removed and changed monitors together A reload that both removes and changes monitors checks that changed monitors, which are in diff.added and diff.changed, don't offset the loss. Co-Authored-By: Claude Opus 5.5 --- monitor/loader_stoploss_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/monitor/loader_stoploss_test.go b/monitor/loader_stoploss_test.go index 98f0c42..6eb519d 100644 --- a/monitor/loader_stoploss_test.go +++ b/monitor/loader_stoploss_test.go @@ -42,6 +42,8 @@ func TestLoaderStopLoss(t *testing.T) { {name: "number above with new", stopLoss: "3", removed: 5, added: 1, stopped: true}, {name: "number replaced", stopLoss: "3", removed: 5, added: 5}, {name: "number changed", stopLoss: "3", changed: 5}, + {name: "number at with changed", stopLoss: "3", removed: 3, changed: 2}, + {name: "number above with changed", stopLoss: "3", removed: 4, changed: 2, stopped: true}, {name: "percent below", stopLoss: "30%", removed: 2}, {name: "percent at", stopLoss: "30%", removed: 3}, {name: "percent above", stopLoss: "30%", removed: 4, stopped: true}, @@ -51,6 +53,8 @@ func TestLoaderStopLoss(t *testing.T) { {name: "percent above with new", stopLoss: "30%", removed: 5, added: 1, stopped: true}, {name: "percent replaced", stopLoss: "30%", removed: 5, added: 5}, {name: "percent changed", stopLoss: "30%", changed: 5}, + {name: "percent at with changed", stopLoss: "30%", removed: 3, changed: 2}, + {name: "percent above with changed", stopLoss: "30%", removed: 4, changed: 2, stopped: true}, {name: "disabled", stopLoss: "", removed: 10}, } for _, tt := range tests { From 72d8b009279a0896e84f84738dc7fe61108af44a Mon Sep 17 00:00:00 2001 From: Ian Oberst Date: Thu, 29 Oct 2026 07:18:10 -0700 Subject: [PATCH 3/3] Check stop-loss event message and unchanged monitors TestLoaderStopLoss now checks the monitors-stoploss event message, which used to print the loss fraction as a percent, and that ErrStopLoss leaves every loaded monitor in place, not just the count. Also clarify the stop-loss comment and docs wording. Co-Authored-By: Claude Opus 5.5 --- docs/content/monitors/loading.md | 2 +- monitor/loader.go | 12 ++++---- monitor/loader_stoploss_test.go | 53 ++++++++++++++++++++++++++------ 3 files changed, 51 insertions(+), 16 deletions(-) diff --git a/docs/content/monitors/loading.md b/docs/content/monitors/loading.md index 85a6b73..e0001aa 100644 --- a/docs/content/monitors/loading.md +++ b/docs/content/monitors/loading.md @@ -54,7 +54,7 @@ Stop-loss prevents reloading from dropping too many MySQL instances due to unrel For example, maybe the `LoadMonitors` plugin has a bug that causes it to return zero monitors when there are really 10 monitors. Stop-loss would prevent Blip from unloading all monitors, which could have adverse side effects for visibility and alerting. -Stop-loss is the net drop in the number of monitors: the number loaded before the reload minus the number that would be loaded after it. +Stop-loss measures the net drop in the number of monitors: the number loaded before the reload minus the number that would be loaded after it. New monitors offset removed monitors, and changed monitors don't count. For example, with 10 monitors loaded, a reload that removes 4 monitors and adds 1 drops 3 monitors. If the drop is greater than the stop-loss limit, the reload fails and the loaded monitors are not changed. diff --git a/monitor/loader.go b/monitor/loader.go index 24b53c5..e21be00 100644 --- a/monitor/loader.go +++ b/monitor/loader.go @@ -312,12 +312,12 @@ func (ml *Loader) loadAndUnload(ctx context.Context) ([]*stoppingMonitor, error) // a big drop in the number because it might be a false-positive that will // set off alarms when a bunch of metrics fail to report. // - // The loss is the net drop in the number of monitors: before is the number - // loaded now, and now is the number that would be loaded after this change. - // New monitors offset removed ones; changed monitors are in both diff.added - // and diff.changed, so they don't change the number. Stop-loss is a number - // or a percent, not both (see blip.StopLoss), and the loss must be greater - // than the limit to trigger it. + // The loss is the net drop in the number of monitors: nBefore is the number + // currently loaded, and nNow is the number that would be loaded after this + // change. New monitors offset removed ones; changed monitors are in both + // diff.added and diff.changed, so they don't change the number. Stop-loss + // is a number or a percent, not both (see blip.StopLoss), and the loss must + // be greater than the limit to trigger it. nBefore := len(ml.repo) nNow := nBefore - len(diff.removed) + len(diff.added) - len(diff.changed) if nNow < nBefore { diff --git a/monitor/loader_stoploss_test.go b/monitor/loader_stoploss_test.go index 6eb519d..2d52871 100644 --- a/monitor/loader_stoploss_test.go +++ b/monitor/loader_stoploss_test.go @@ -14,6 +14,7 @@ import ( "github.com/cashapp/blip/v2" "github.com/cashapp/blip/v2/aws" "github.com/cashapp/blip/v2/dbconn" + "github.com/cashapp/blip/v2/event" "github.com/cashapp/blip/v2/monitor" "github.com/cashapp/blip/v2/plan" "github.com/cashapp/blip/v2/test/mock" @@ -31,34 +32,52 @@ func TestLoaderStopLoss(t *testing.T) { removed int added int changed int - stopped bool // expect ErrStopLoss + stopped bool // expect ErrStopLoss + msg string // expected monitors-stoploss event message }{ {name: "number below", stopLoss: "3", removed: 2}, {name: "number at", stopLoss: "3", removed: 3}, - {name: "number above", stopLoss: "3", removed: 4, stopped: true}, - {name: "number all", stopLoss: "3", removed: 10, stopped: true}, + {name: "number above", stopLoss: "3", removed: 4, stopped: true, msg: "before: 10; now: 6; lost 4 > limit 3"}, + {name: "number all", stopLoss: "3", removed: 10, stopped: true, msg: "before: 10; now: 0; lost 10 > limit 3"}, {name: "number below with new", stopLoss: "3", removed: 3, added: 1}, {name: "number at with new", stopLoss: "3", removed: 4, added: 1}, - {name: "number above with new", stopLoss: "3", removed: 5, added: 1, stopped: true}, + {name: "number above with new", stopLoss: "3", removed: 5, added: 1, stopped: true, msg: "before: 10; now: 6; lost 4 > limit 3"}, {name: "number replaced", stopLoss: "3", removed: 5, added: 5}, {name: "number changed", stopLoss: "3", changed: 5}, {name: "number at with changed", stopLoss: "3", removed: 3, changed: 2}, - {name: "number above with changed", stopLoss: "3", removed: 4, changed: 2, stopped: true}, + {name: "number above with changed", stopLoss: "3", removed: 4, changed: 2, stopped: true, msg: "before: 10; now: 6; lost 4 > limit 3"}, {name: "percent below", stopLoss: "30%", removed: 2}, {name: "percent at", stopLoss: "30%", removed: 3}, - {name: "percent above", stopLoss: "30%", removed: 4, stopped: true}, - {name: "percent all", stopLoss: "30%", removed: 10, stopped: true}, + {name: "percent above", stopLoss: "30%", removed: 4, stopped: true, msg: "before: 10; now: 6; lost 40.0% > limit 30%"}, + {name: "percent all", stopLoss: "30%", removed: 10, stopped: true, msg: "before: 10; now: 0; lost 100.0% > limit 30%"}, {name: "percent below with new", stopLoss: "30%", removed: 3, added: 1}, {name: "percent at with new", stopLoss: "30%", removed: 4, added: 1}, - {name: "percent above with new", stopLoss: "30%", removed: 5, added: 1, stopped: true}, + {name: "percent above with new", stopLoss: "30%", removed: 5, added: 1, stopped: true, msg: "before: 10; now: 6; lost 40.0% > limit 30%"}, {name: "percent replaced", stopLoss: "30%", removed: 5, added: 5}, {name: "percent changed", stopLoss: "30%", changed: 5}, {name: "percent at with changed", stopLoss: "30%", removed: 3, changed: 2}, - {name: "percent above with changed", stopLoss: "30%", removed: 4, changed: 2, stopped: true}, + {name: "percent above with changed", stopLoss: "30%", removed: 4, changed: 2, stopped: true, msg: "before: 10; now: 6; lost 40.0% > limit 30%"}, {name: "disabled", stopLoss: "", removed: 10}, } + var evmux sync.Mutex + var stoplossEvents []event.Event + event.Subscribe(mock.EventReceiver{ + RecvFunc: func(e event.Event) { + if e.Event == event.MONITORS_STOPLOSS { + evmux.Lock() + stoplossEvents = append(stoplossEvents, e) + evmux.Unlock() + } + }, + }) + t.Cleanup(event.RemoveSubscribers) + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + evmux.Lock() + stoplossEvents = nil + evmux.Unlock() + var mux sync.Mutex configs := make([]blip.ConfigMonitor, nBefore) for i := range configs { @@ -76,6 +95,10 @@ func TestLoaderStopLoss(t *testing.T) { ml.Unload(mon.MonitorId(), true) } }() + loaded := map[string]*monitor.Monitor{} + for _, mon := range ml.Monitors() { + loaded[mon.MonitorId()] = mon + } // Remove the first monitors, change the next ones, and add new ones. mux.Lock() @@ -90,12 +113,24 @@ func TestLoaderStopLoss(t *testing.T) { mux.Unlock() err := ml.Load(context.Background()) + evmux.Lock() + events := stoplossEvents + evmux.Unlock() if tt.stopped { assert.ErrorIs(t, err, monitor.ErrStopLoss) + // No monitor is removed, replaced, or added. assert.Equal(t, uint(nBefore), ml.Count(), "monitors changed after ErrStopLoss") + for id, mon := range loaded { + assert.Same(t, mon, ml.Monitor(id), "monitor %s replaced after ErrStopLoss", id) + } + if assert.Len(t, events, 1) { + assert.True(t, events[0].Error) + assert.Equal(t, tt.msg, events[0].Message) + } } else { assert.NoError(t, err) assert.Equal(t, uint(nBefore-tt.removed+tt.added), ml.Count()) + assert.Empty(t, events) } }) }