fix: update endsAt when resolving alerts via API #4717
Closed
+216
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
I believe this behavior is intentional, see here. |
ultrotter
suggested changes
Nov 10, 2025
types/types.go
Outdated
| if o.Resolved() { | ||
| // If the new alert is resolved, use its EndsAt timestamp. | ||
| // This handles the case where an alert is being resolved via API. | ||
| res.EndsAt = o.EndsAt |
Contributor
There was a problem hiding this comment.
At line 448 we already do res := o, so res.EndsAt should already be o.EndsAt?
provider/mem/mem.go
Outdated
| // the new alert is updating the existing alert (e.g., setting endsAt | ||
| // to resolve it, or updating annotations/labels). | ||
| hasOverlap := (alert.EndsAt.After(old.StartsAt) && alert.EndsAt.Before(old.EndsAt)) || | ||
| (alert.StartsAt.After(old.StartsAt) && alert.StartsAt.Before(old.EndsAt)) |
Contributor
There was a problem hiding this comment.
@grobinson-grafana in this case should alert.StartsAt be After old.StartsAt, or rather
!alert.StartsAt.Before(old.StartsAt) && alert.StartsAt.Before(old.EndsAt))
This would fix StartsAt being the same (which is overlapping, if I'm correct...)
provider/mem/mem.go
Outdated
|
|
||
| // Also merge if updating an existing alert (same fingerprint) to allow | ||
| // updates like setting endsAt to resolve an alert, even without overlap. | ||
| isUpdate := !alert.UpdatedAt.Before(old.UpdatedAt) |
Contributor
There was a problem hiding this comment.
This might be a bit too broad, as one could just set UpdatedAt to now to make any possible alert updates?
tennisleng
added a commit
to tennisleng/alertmanager
that referenced
this pull request
Nov 11, 2025
- 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 prometheus#4717
provider/mem/mem.go
Outdated
| // This handles the case where an alert is resolved via API without time overlap. | ||
| // An alert is being resolved if it has endsAt set and the old alert doesn't, | ||
| // or if the new endsAt is earlier than the old endsAt. | ||
| isResolving := !alert.EndsAt.IsZero() && (old.EndsAt.IsZero() || alert.EndsAt.Before(old.EndsAt)) |
Collaborator
There was a problem hiding this comment.
I am struggling to figure out if this is correct or not. It doesn't seem correct to me that we just merge an old alert regardless of its timestamps?
tennisleng
force-pushed
the
fix/endsat-not-updated
branch
2 times, most recently
from
4213987 to
2661448
Compare
When an alert is posted via the API with only endsAt set (to resolve an existing alert), the endsAt field was not being updated due to two issues: 1. The merge condition in provider/mem/mem.go only merged alerts when there was an overlap in activity range. This prevented merging when resolving an alert (where the new endsAt is before the old endsAt timeout). 2. The Merge function in types/types.go didn't properly handle the case where a new resolved alert is merged with an existing unresolved alert. This fix: - Updates the merge condition to also merge when updating an existing alert (same fingerprint), allowing updates like setting endsAt to resolve an alert even without overlap. - Updates the Merge function to always use the new alert's EndsAt when it is resolved, ensuring that resolving an alert via API properly updates the endsAt timestamp. Fixes prometheus#4554 Signed-off-by: tennisleng <tennisleng@users.noreply.github.com>
- 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 prometheus#4717 Signed-off-by: Andrew Leng <work@Andrews-MacBook-Air.local>
- Add StartsAt equality check to isResolving condition to ensure only alerts with identical start times can be merged for resolution - This prevents unintended merges of alerts that appear similar but represent different alert instances - Add comprehensive unit tests covering all merge scenarios: * Resolving active alerts via API * Re-resolving already resolved alerts to earlier times * Rejecting merges with different StartsAt (security) * Rejecting attempts to extend active alerts (security) Addresses reviewer concern about overly permissive merge logic that could allow merging alerts 'regardless of timestamps'. Signed-off-by: Andrew Leng <work@Andrews-MacBook-Air.local>
tennisleng
force-pushed
the
fix/endsat-not-updated
branch
from
2661448 to
ffea952
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes issue #4554 where was not being updated when resolving alerts via the API.
Problem
When posting an alert via the API with only set (to resolve an existing alert), the field was not being updated. This happened due to two issues:
Merge condition too restrictive: The merge logic in only merged alerts when there was an overlap in activity range. When resolving an alert, the new (in the past) doesn't overlap with the old (future timeout), so no merge occurred.
Merge function bug: The function in didn't properly handle the case where a new resolved alert is merged with an existing unresolved alert.
Solution
Updated merge condition: Modified to also merge alerts when updating an existing alert (same fingerprint), allowing updates like setting to resolve an alert even without overlap.
Fixed Merge function: Updated to always use the new alert's when it is resolved, ensuring that resolving an alert via API properly updates the timestamp.
Testing
Type of Change
Fixes #4554