From 3d74cd9431a48fa89c6489689eac82a872799ea0 Mon Sep 17 00:00:00 2001 From: naiba Date: Mon, 8 Jun 2026 00:57:14 +0000 Subject: [PATCH] fix(alert): retain Duration samples for offline rules so they fire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RetentionWindow() trimmed offline-rule samples to 1, but Check's offline branch reads points[len-Duration:] and needs Duration samples. The window never filled, boundCheck always returned "passed", and offline alerts never fired a notification — while other alert types worked. Offline rules now retain Duration samples (cycle rules still keep 1, matching their last-sample lookback). Fixes the prior over-correction in RetentionWindow; corrects the test that had pinned the buggy offline=1 value, and adds offline + combined-rule regression tests driving the real trim loop. --- model/alertrule.go | 5 ++- model/alertrule_test.go | 68 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/model/alertrule.go b/model/alertrule.go index e47668b2..37e734c6 100644 --- a/model/alertrule.go +++ b/model/alertrule.go @@ -199,11 +199,14 @@ func (r *AlertRule) Check(points [][]bool) (int, bool) { // RetentionWindow 返回保留历史采样所需的长度（各规则窗口的最大值），只依赖 // 规则定义而非 Check 的判定结果——否则窗口未填满时 Check 返回的 max=0 会被 // 误判为"无需历史"而清空采样，使规则永远攒不够样本。 +// 各规则类型回看的采样数必须与 Check 中实际读取的窗口一致： +// - 周期流量规则：Check 只读最后 1 个采样点 → 需要 1 +// - 离线规则、常规规则：Check 读取 points[len-Duration:] → 需要 Duration func (r *AlertRule) RetentionWindow() int { window := 0 for _, rule := range r.Rules { var need int - if rule.IsTransferDurationRule() || rule.IsOfflineRule() { + if rule.IsTransferDurationRule() { need = 1 } else if d := int(rule.Duration); d > 0 { need = d diff --git a/model/alertrule_test.go b/model/alertrule_test.go index 393c16bc..72cd18df 100644 --- a/model/alertrule_test.go +++ b/model/alertrule_test.go @@ -415,7 +415,7 @@ func TestAlertRule_RetentionWindow(t *testing.T) { {"single general", &AlertRule{Rules: []*Rule{{Type: "cpu", Duration: 10}}}, 10}, {"zero duration only", &AlertRule{Rules: []*Rule{{Type: "cpu", Duration: 0}}}, 0}, {"mixed picks max", &AlertRule{Rules: []*Rule{{Type: "cpu", Duration: 0}, {Type: "cpu", Duration: 7}}}, 7}, - {"offline looks back one", &AlertRule{Rules: []*Rule{{Type: "offline", Duration: 30}}}, 1}, + {"offline keeps Duration", &AlertRule{Rules: []*Rule{{Type: "offline", Duration: 30}}}, 30}, {"cycle looks back one", &AlertRule{Rules: []*Rule{{Type: "net_in_speed_cycle"}}}, 1}, } for _, c := range cases { @@ -424,3 +424,69 @@ func TestAlertRule_RetentionWindow(t *testing.T) { } } } + +// TestAlertRule_OfflineRuleAccumulatesSamples is a regression guard for offline +// alerts that never fire. Check's offline branch reads points[len-Duration:], +// so it needs Duration samples retained; if RetentionWindow trims to 1 (as an +// earlier fix wrongly did for offline rules), the window never reaches Duration, +// boundCheck keeps returning "passed", and the offline alert never raises. +func TestAlertRule_OfflineRuleAccumulatesSamples(t *testing.T) { + const duration = 10 + rule := &AlertRule{Rules: []*Rule{{Type: "offline", Duration: duration}}} + + var samples [][]bool + var lastPassed bool + maxLen := 0 + for tick := 0; tick < duration*3; tick++ { + samples = append(samples, []bool{false}) // offline sample + _, lastPassed = rule.Check(samples) + samples = trimSamples(samples, rule.RetentionWindow()) + if len(samples) > maxLen { + maxLen = len(samples) + } + } + + if maxLen < duration { + t.Fatalf("offline samples never accumulated to Duration: max window reached %d, want >= %d", maxLen, duration) + } + if lastPassed { + t.Fatalf("a server offline every tick must eventually fail the offline check (passed=false), got passed=true") + } +} + +// TestAlertRule_CombinedRuleAccumulatesSamples drives the real trim loop for +// mixed-type alerts. The verdict is AND-of-failure: an incident fires only once +// every rule's lookback window is full and all fail. RetentionWindow must keep +// enough samples for the largest window (offline/general need Duration, cycle +// needs 1); if any rule type is under-retained the alert never fires. +func TestAlertRule_CombinedRuleAccumulatesSamples(t *testing.T) { + cases := []struct { + msg string + rule *AlertRule + sample []bool + fireAt int // tick index where passed must first become false + wantWindow int + }{ + {"general3+general10", &AlertRule{Rules: []*Rule{{Type: "cpu", Duration: 3}, {Type: "memory", Duration: 10}}}, []bool{false, false}, 9, 10}, + {"offline5+general10", &AlertRule{Rules: []*Rule{{Type: "offline", Duration: 5}, {Type: "cpu", Duration: 10}}}, []bool{false, false}, 9, 10}, + {"transfer+general8", &AlertRule{Rules: []*Rule{{Type: "net_in_speed_cycle"}, {Type: "cpu", Duration: 8}}}, []bool{false, false}, 7, 8}, + {"offline3+offline12", &AlertRule{Rules: []*Rule{{Type: "offline", Duration: 3}, {Type: "offline", Duration: 12}}}, []bool{false, false}, 11, 12}, + } + for _, c := range cases { + if got := c.rule.RetentionWindow(); got != c.wantWindow { + t.Fatalf("%s: RetentionWindow()=%d want %d", c.msg, got, c.wantWindow) + } + var samples [][]bool + firstFire := -1 + for tick := 0; tick < c.wantWindow*3; tick++ { + samples = append(samples, append([]bool(nil), c.sample...)) + if _, passed := c.rule.Check(samples); !passed && firstFire < 0 { + firstFire = tick + } + samples = trimSamples(samples, c.rule.RetentionWindow()) + } + if firstFire != c.fireAt { + t.Fatalf("%s: alert first fired at tick %d, want %d (never-firing = -1)", c.msg, firstFire, c.fireAt) + } + } +}