Skip to content

Commit dd21d43

Browse files
tennislengAndrew Leng
authored andcommitted
fix: address PR review comments
- Remove redundant EndsAt assignment in types/types.go (res is already a copy of o) - Fix overlap check to handle equal StartsAt timestamps using !Before instead of After - Replace isUpdate check with more specific isResolving check that only allows merging when an alert is being resolved (endsAt set and old alert doesn't have it, or new endsAt is earlier than old endsAt), preventing arbitrary updates based solely on UpdatedAt timestamp Fixes review comments from #4717 Signed-off-by: Andrew Leng <work@Andrews-MacBook-Air.local>
1 parent b5ff56e commit dd21d43

File tree

2 files changed

+13
-9
lines changed

2 files changed

+13
-9
lines changed

provider/mem/mem.go

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -243,16 +243,20 @@ func (a *Alerts) Put(alerts ...*types.Alert) error {
243243
existing = true
244244

245245
// Merge alerts if there is an overlap in activity range, or if
246-
// the new alert is updating the existing alert (e.g., setting endsAt
247-
// to resolve it, or updating annotations/labels).
248-
hasOverlap := (alert.EndsAt.After(old.StartsAt) && alert.EndsAt.Before(old.EndsAt)) ||
249-
(alert.StartsAt.After(old.StartsAt) && alert.StartsAt.Before(old.EndsAt))
246+
// the new alert is resolving the existing alert.
247+
// Overlap check: check if alert's time range overlaps with old's time range.
248+
// Handle equal timestamps as overlapping (use !Before instead of After).
249+
// An alert with zero EndsAt is considered active indefinitely.
250+
hasOverlap := (!old.EndsAt.IsZero() && !alert.EndsAt.Before(old.StartsAt) && alert.EndsAt.Before(old.EndsAt)) ||
251+
(!alert.StartsAt.Before(old.StartsAt) && (old.EndsAt.IsZero() || alert.StartsAt.Before(old.EndsAt)))
250252

251-
// Also merge if updating an existing alert (same fingerprint) to allow
252-
// updates like setting endsAt to resolve an alert, even without overlap.
253-
isUpdate := !alert.UpdatedAt.Before(old.UpdatedAt)
253+
// Merge if the new alert is resolving the old alert (setting endsAt to resolve it).
254+
// This handles the case where an alert is resolved via API without time overlap.
255+
// An alert is being resolved if it has endsAt set and the old alert doesn't,
256+
// or if the new endsAt is earlier than the old endsAt.
257+
isResolving := !alert.EndsAt.IsZero() && (old.EndsAt.IsZero() || alert.EndsAt.Before(old.EndsAt))
254258

255-
if hasOverlap || isUpdate {
259+
if hasOverlap || isResolving {
256260
alert = old.Merge(alert)
257261
}
258262
}

types/types.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -455,7 +455,7 @@ func (a *Alert) Merge(o *Alert) *Alert {
455455
if o.Resolved() {
456456
// If the new alert is resolved, use its EndsAt timestamp.
457457
// This handles the case where an alert is being resolved via API.
458-
res.EndsAt = o.EndsAt
458+
// Note: res.EndsAt is already o.EndsAt since res := *o above.
459459
// The latest explicit resolved timestamp wins if both alerts are effectively resolved.
460460
if a.Resolved() && a.EndsAt.After(o.EndsAt) {
461461
res.EndsAt = a.EndsAt

0 commit comments

Comments
 (0)