[bugfix] Alarm group convergence must not resolve while members are still firing

[nikhiln64]

Fixes #4160.

The alarm group convergence path in AlarmGroupReduce decides a group's status from the wrong set of alerts, and because of that it does two bad things. It flips a group to resolved while other members are still firing, and it silently throws away recovery events so the database record stays firing forever. Both come from the same place. The per group cache, cache.getAlertFingerprints(), is fully cleared after every send, so by the time the code asks whether the group is firing or resolved it is only looking at the alerts that happened to arrive during the current send window rather than at every member that is still active.

The first symptom is a premature resolve. After a send that carried both CPU firing and Memory firing, the cache is emptied. When CPU recovers a lone CPU resolved lands in that emptied cache, shouldSendGroupImmediately sees a cache where everything is resolved, and the whole group goes out as resolved even though Memory never stopped firing. You can see it in the payloads where the resolved group push is immediately followed by another Memory firing push, which would be impossible if Memory had actually recovered.

The second symptom is a lost recovery. After a firing send arms the repeat interval, the cache is cleared again. CPU keeps firing and Memory recovers, so the cache now holds a firing CPU next to a resolved Memory. determineGroupStatus returns firing because CPU is firing, the repeat interval throttle in sendGroupAlert returns early, and then runCheckAndSendGroups clears the cache anyway. The Memory resolved transition is gone. It is never emitted, the plugin never sees it, and the alert stays firing in the database with no endAt and no auto resolve.

The fix is to stop treating the cache as a per window scratch buffer and let it hold the live set of members. This change keeps firing alerts across sends instead of clearing everything, removes only the resolved members after they have actually been emitted, and makes sure the firing repeat interval throttle suppresses repeated firing notifications only and never a pending resolved transition. With that in place the group stays firing while any member is still active, a member recovery rides out inside the still firing group, and the group only resolves once every member has genuinely cleared.

I added two regression tests in AlarmGroupReduceTest. One asserts that no emitted group ever carries resolved status while a member is still firing, the other asserts that a member recovery inside the repeat interval window is still emitted. Both fail on the current code and pass with this change, and the affected module builds green.

One honest note. This is timing sensitive and not perfectly reproducible from the outside, but the code paths are deterministic and the fix follows directly from them. I deliberately left two pre existing behaviours out of scope to keep the change focused, that lastRepeatTime is not reset when a group fully drains, and that empty group caches are never removed from groupCacheMap. Happy to look at either separately if you would like.

[Aias00]

Review: [bugfix] Alarm group convergence must not resolve while members are firing (#4160)

Verdict: ✅ APPROVED — correct fix, strong regression tests.

What this PR does

Fixes two related convergence bugs where a group could be wrongly resolved or a recovery could be dropped:

• runCheckAndSendGroups no longer clears cache.getAlertFingerprints() after each send. The cache now retains every currently-active member, so group status is computed over the full member set instead of only the alerts received in the current send window.
• processAlertByGroupDefine no longer early-returns on an existing fingerprint; it always refreshes startAt and re-puts, and (via the removed early return) now also re-evaluates shouldSendGroupImmediately, so a member transitioning to resolved triggers a send promptly.
• sendGroupAlert computes hasResolvedAlert; the firing repeat-interval throttle now only suppresses repeated firing notifications and never swallows a pending recovery. After emitting, only the resolved members are pruned — firing members are kept until they recover.

Assessment

• Correctness: The group-status decision now reflects all live members, so "one recovers, another still firing" keeps the group firing — exactly the intended behavior. Ordering is right: the GroupAlert is built (including resolved members) before they are pruned, so the recovery is still communicated.
• Tests: Two regression tests precisely reproduce #4160 Bug1 (group must not resolve while a member still fires) and Bug2 (recovery inside the repeat-interval window must still be emitted). Good coverage.

Concerns (non-blocking)

1. Unbounded firing-member retention: fingerprints are now only removed when a member is explicitly resolved. A member that fires and never receives a resolved event stays in the cache indefinitely — this can keep a group perpetually "firing" and grow the cache. Please confirm whether silent members (alert stops being reported without a resolve event) are evicted elsewhere, or consider adding a max-age/TTL eviction for GroupAlertCache entries.
2. Visibility change: runCheckAndSendGroups was made package-private (dropped private) to allow direct test invocation. Acceptable for testing, but a small test hook / visible-for-testing annotation would be cleaner than widening visibility.

No blocking issues. Solid fix.

[nikhiln64]

Thanks for the thorough review @Aias00, glad the fix and the tests read well. You are right on both notes. The unbounded retention is a real gap, though it is an existing property of GroupAlertCache rather than something this change introduced, since a silently stopped member and an emptied group cache were never evicted before either. I would rather not widen the scope of this bugfix, so I am happy to follow up with a max age eviction for the cache entries in a separate PR, so a member that stops being reported without a resolve event cannot keep a group firing forever. On the visibility, hertzbeat does not use a VisibleForTesting annotation anywhere in the tree today, so I left runCheckAndSendGroups package private with the explaining comment rather than pull in a new annotation, though I can switch it to whatever hook you prefer.

The Backend CI and License Checker runs are sitting in action_required and need a maintainer to approve them before they start, so whenever you have a moment could you approve the run so this can go green for merge. Happy to rebase onto the latest master first if you would prefer the branch current.

[Aias00]

LGTM, thx for your contribution