fix(exporters): keep limiting after the first dropped alert

[AlonLiwsky]

Summary

The per-minute alert limit stops limiting after its first drop, so it drops exactly one alert
per minute and sends every later one. In practice there is no limit.

shouldSendLimitAlert returns count > max && !isNotified — one value carrying two decisions.
The first alert past the limit is dropped and sets isNotified, which makes the condition false
for every later alert in that minute, so they are all sent.

Ticket

None — this repository has no ticket link.

Changes

• admitAlert replaces shouldSendLimitAlert and returns the two decisions separately:
  admitted is false for every alert past the limit, notify is true only for the first one.
• Both call sites drop the alert whenever it is not admitted, and send the AlertLimitReached
  notice only on the first. ReportAlertSuppressed(..., "rate_limit") now fires for every
  dropped alert instead of once per minute, so that metric becomes usable.
• A limit of zero or less means no limit. Validate() already replaces a zero with the default,
  so this only guards an exporter built by hand.
• The window reset no longer swallows the alert that triggers it. The old code returned early
  without counting, so the effective limit was max + 1.
• sendAlertLimitReached no longer sets the notified flag — admitAlert owns that decision —
  and no longer reads count and startTime without the lock.
• New docs/features/alert-rate-limit.md.

Testing

go build ./pkg/exporters/ for GOOS=linux → pass. New tests in
pkg/exporters/alert_limit_test.go: the limiter keeps limiting after its first drop, the window
resets, zero means no limit, and twenty concurrent senders admit exactly the limit while the
notice is decided once. Run on Linux with -race → pass, together with the existing
TestSendRuleAlertRateReached.

AI Review

Local review with armosec-shared-rules:code-review-standards (Opus, high effort) over the
sibling change in the private agent, which carries the identical fix. Verdict: the limiter logic
is correct, the mutex covers the whole decision, and all call sites are consistent. Its one
finding was about the consequence rather than the code — a real limit can starve one alert kind
when consumers share an exporter — which is why ReportAlertSuppressed now counts every drop.

AI-skills: armosec-shared-rules:agent-dispatch-policy

Summary by CodeRabbit

• Bug Fixes

  • Improved alert rate limiting so alerts beyond the configured per-minute limit are consistently suppressed.
  • Ensured only one limit-reached notification is emitted per rate-limit window.
  • Added reliable window resets and concurrency-safe enforcement.
  • Nonpositive limits continue to allow unlimited alerts.

• Documentation

  • Added documentation covering rate-limit behavior, notifications, metrics, and configuration considerations.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 86259185-ad40-47b2-a187-e904f37fe5fa

📥 Commits

Reviewing files that changed from the base of the PR and between 1bbe089 and 7571aae.

📒 Files selected for processing (3)

• docs/features/alert-rate-limit.md
• pkg/exporters/alert_limit_test.go
• pkg/exporters/http_exporter.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The HTTP exporter now enforces MaxAlertsPerMinute for every alert, emits one limit-reached notification per window, and tracks suppressed alerts. Tests cover resets, unlimited limits, and concurrent senders. Documentation describes the behavior and fixed-window constraints.

Changes

Alert rate-limit enforcement

Layer / File(s)	Summary
Admission logic and alert send paths pkg/exporters/http_exporter.go	admitAlert denies alerts after the configured limit and returns a separate notification flag. Rule and malware alert paths send the limit-reached alert only for the first denial. Logging reads rate-limit state under the mutex.
Behavior validation and documented limits pkg/exporters/alert_limit_test.go, docs/features/alert-rate-limit.md	Tests cover repeated suppression, window resets, unlimited limits, and concurrent senders. Documentation describes admission results, suppression metrics, and fixed-window behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🔵 Low · up to 7571a

The per-minute limit now suppresses every alert beyond the configured cap, and rule and malware alerts share that quota; a burst from one category can temporarily suppress alerts from the other until the window resets. The PR is mergeable with explicit owner awareness of this bounded tradeoff.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: continuing alert rate limiting after the first dropped alert.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[matthyx]

Reviewed the logic in pkg/exporters/http_exporter.go independently of the PR description's own AI-review claim.

Correctness: looks right. Traced admitAlert() by hand against the bug it fixes:

• Old code: count > max && !isNotified conflated "drop this alert" and "send the notice" into one bool. The first drop set isNotified, which flipped the condition to false for every later alert in the window — so the limiter dropped exactly one alert per minute and let the rest through.
• New code separates admitted (false for every send past the limit) from notify (true only once, on the first refusal), decided under alertMetrics's mutex for the whole operation. Verified: window reset now counts the triggering alert (fixes the old max+1 effective limit), sendAlertLimitReached no longer double-sets isNotified or reads count/startTime unlocked, and MaxAlertsPerMinute <= 0 correctly short-circuits to "no limit" (matches Validate()'s zero→default behavior for the common path, and still guards a hand-built exporter with a negative value).
• Existing TestSendRuleAlertRateReached still passes under the new semantics (traced by hand: 1st call admitted, 2nd call not admitted + notify). New tests in alert_limit_test.go cover the keep-limiting-after-drop behavior, window reset, zero-means-no-limit, and a 20×20 concurrent-sender case pinning admitted==limit and notified==1 — good coverage for a mutex-guarded counter.
• Confirmed no other call sites reference the old shouldSendLimitAlert name (searched main); the rename is clean.

Blockers before this can merge:

1. DCO check is failing (action_required). The commit 444b0432 has no Signed-off-by trailer. This needs git commit --amend -s (and a force-push) to pass.
2. PR is still a draft — needs to be marked "Ready for review" before it's mergeable regardless of review state.

No code changes requested — once the DCO sign-off is added and the PR is marked ready, this looks good to merge from a correctness standpoint.

[matthyx]

Re-checked: both blockers from my previous review are resolved.

• DCO now passes — the commit carries a Signed-off-by trailer.
• PR is no longer a draft.

Code is unchanged since my last pass (http_exporter.go diff is identical) and I already traced admitAlert() by hand against the original bug — the fix is correct, the mutex covers the whole decision, and the new tests (including the concurrent-sender case) exercise the right behavior. CodeRabbit's automated review also came back clean with no actionable comments.

Approving.