Fix/auto resolve grouped alerts

[Aryainguz]

The Bug

When an alert with a Group By clause evaluates multiple time windows (buckets) in a single cron execution tick, if the first bucket triggered an ALERT, but the last bucket did not exceed the threshold, the alert history state was never explicitly reset to OK inside the evaluation loop.

Fixes #2623

As a result, the alert history retained the ALERT state for that tick, failing to trigger the sendNotificationIfResolved logic. The alert remained permanently stuck in the ALERT state even after the underlying metrics returned to normal levels. (See the original developer TODO at packages/api/src/tasks/checkAlerts/index.ts:1293 calling out this edge case).

The Fix

Implemented the explicit state reset inside the threshold evaluation else block:

} else {
  // If the threshold is not met, reset the state to OK.
  // This ensures that if a previous window in this evaluation triggered an ALERT,
  // a subsequent OK window correctly resolves it before the notification phase.
  history.state = AlertState.OK;
  history.counts = 0;
}

Verification

• Updated checkAlerts tests.
• Code correctly resets the history state to OK.
• Tested against the checkAlerts test suite.
• Changeset created (@hyperdx/api: patch).

Local Validations :

[github-actions]

Deep Review

✅ No critical issues found. The core state-machine fix is sound: shouldFireBasedOnConsecutiveWindows reads only cross-tick history (not the mutated per-tick counts), so the new else-branch reset does not corrupt multi-window accumulation; ALERT sends are correctly collapsed to one-per-group-per-tick; and the webhook-delete guard change is a safe no-op given channel.webhookId only exists on type:'webhook' channels. The findings below are recommendations and nits.

🟡 P2 — recommended

• packages/api/src/tasks/checkAlerts/index.ts:1365 — A group that breaches and recovers within the same tick fires an ALERT immediately followed by a RESOLVED notification; a continuously flapping metric therefore emits two notifications every tick indefinitely, since firing is gated on latestAlertContext having an entry rather than on the group's final state.
  • Fix: Confirm this pairing is the intended product behavior, and if flapping noise is a concern, suppress the deferred ALERT when the group's final tick state is OK.
  • _{adversarial, reliability, maintainability}
• packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.test.ts:8534 — The new same-tick breach-then-recover test uses a non-grouped saved search, so the grouped (groupBy) path that the change specifically targets has no coverage for per-group isolation of ALERT+RESOLVED.
  • Fix: Add a grouped-alert test where one group breaches-then-recovers in a tick while a sibling group stays purely OK or purely ALERT, asserting per-group state and notification payloads.
  • _{testing, correctness, adversarial}
• packages/api/src/routers/api/webhooks.ts:372 — The added 'channel.type': 'webhook' condition on the delete-reference count query has no test locking in its intended behavior.
  • Fix: Add a webhooks DELETE test covering an alert that references the webhook id under a non-webhook channel type versus one that references it as a webhook channel.
  • _{testing, project-standards}
• packages/api/src/tasks/checkAlerts/index.ts:1314 — The else reset sets history.state = OK and counts = 0 but leaves history.fired unchanged, so an earlier breaching bucket's fired: true persists on the OK record; with numConsecutiveWindows > 1 this can carry forward into a later PENDING window and produce a RESOLVED notification with no preceding ALERT.
  • Fix: Reset history.fired = false alongside the state/counts reset in the else branch.
  • _{reliability, kieran-typescript, project-standards}
• packages/api/src/tasks/checkAlerts/index.ts:1378 — The synthetic groupPrevious is built by spreading a partial object and casting as AggregatedAlertHistory, fabricating a value that omits required fields (_id, createdAt); it is safe only because sendNotificationIfResolved currently reads just state and fired, and a future field read would be silently undefined.
  • Fix: Narrow the sendNotificationIfResolved parameter to Pick<AggregatedAlertHistory, 'state' | 'fired'> (or pass an explicit boolean flag) to remove the unsafe cast.
  • _{kieran-typescript, maintainability}

🔵 P3 nitpicks (6)

• packages/api/src/tasks/checkAlerts/index.ts:1362 — const wasAlertingBefore is computed but never read.
  • Fix: Delete the unused variable.
  • _{correctness, maintainability, kieran-typescript, project-standards}
• packages/api/src/tasks/checkAlerts/index.ts:1315 — Resetting counts to 0 on a trailing OK bucket makes the persisted AlertHistory.counts reflect only trailing consecutive breaches, so the history UI shows 0 for a tick that had multiple breaching buckets.
  • Fix: If counts is meant to represent breaches-in-window, track a separate max/total instead of resetting the persisted field.
  • _adversarial
• packages/api/src/tasks/checkAlerts/index.ts:1157 — Firing logic is now duplicated across three divergent mechanisms: single_value calls trySendNotification inline while the empty-bucket and data-bucket paths defer via latestAlertContext.
  • Fix: Extract a shared evaluate-and-record helper so firing semantics live in one place.
  • _{maintainability}
• packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.test.ts:8619 — The new test adds an as any cast on connection.id that no other processAlertAtTime call site in the file uses.
  • Fix: Drop the cast to match the established call pattern.
  • _{kieran-typescript}
• packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.test.ts:8611 — Notification payloads are asserted only via loose JSON.stringify substring checks.
  • Fix: Assert on specific structured fields (group key, breaching value, resolved count).
  • _testing
• packages/api/src/routers/api/webhooks.ts:363 — The webhook-delete guard fix is an unrelated logical change bundled into a grouped-alerts PR, against the repo's single-logical-change PR-hygiene guidance.
  • Fix: Split the webhook-guard change into its own PR and changeset.
  • _{project-standards}

---

Reviewers (8): correctness, adversarial, security, testing, reliability, kieran-typescript, maintainability, project-standards.

Testing gaps:

• No grouped-alert (groupBy) coverage for same-tick breach-then-recover — the change's headline scenario.
• No coverage of numConsecutiveWindows > 1 combined with the new else-branch OK reset and fired carry-forward.
• No coverage of multiple ClickHouse rows sharing one group key within a single bucket — the exact case bucketEvaluations worst-case tracking was added to handle.
• No test for the webhooks.ts channel.type filter change.

[brandon-pereira]

Thanks for the contribution, overall LGTM!

One change to flag during testing: Same-tick breach-then-recover fires two notifications

When one evaluation tick backfills multiple buckets and a group breaches in an early bucket but recovers by the final bucket, the run sends two notifications: an ALERT (Triggering ... alarm!) immediately followed by a RESOLVED (Alert resolved ...). The final persisted state is correctly OK, this is only about notification volume.

I actually think the two-notification behaviour is the right default here, silently auto-resolving would hide that a threshold was crossed at all, and "it breached at 22:05 and has since recovered" is useful for on-call awareness and postmortems. So let's keep it as-is.

The one thing I'd ask before merge: since this is now intentional (and easy to accidentally regress into a silent resolve later), let's pin it with a regression test that asserts both the count and the order:

it('same-tick breach-then-recover sends an alert followed by a resolved notification', async () => {
  // setup: threshold > 2, prior OK history at 22:05, now = 22:18
  //   buckets: [22:05-22:10] 3 errors (breach), [22:10-22:15] 1 error (ok)
  await processAlertAtTime(/* ... */);

  expect((await Alert.findById(details.alert.id))!.state).toBe('OK');
  expect(slack.postMessageToWebhook).toHaveBeenCalledTimes(2);
  // assert order: first call ALERT, second RESOLVED
});

Open to being talked out of it though - if you feel two back-to-back messages read as flapping and would rather collapse to a single ALERT that notes the value is already back to normal, I'm fine with that direction too. But absent a strong case, let's keep both + add the test.

[Aryainguz]

Thanks for the contribution, overall LGTM!

One change to flag during testing: Same-tick breach-then-recover fires two notifications

When one evaluation tick backfills multiple buckets and a group breaches in an early bucket but recovers by the final bucket, the run sends two notifications: an ALERT (Triggering ... alarm!) immediately followed by a RESOLVED (Alert resolved ...). The final persisted state is correctly OK, this is only about notification volume.

I actually think the two-notification behaviour is the right default here, silently auto-resolving would hide that a threshold was crossed at all, and "it breached at 22:05 and has since recovered" is useful for on-call awareness and postmortems. So let's keep it as-is.

The one thing I'd ask before merge: since this is now intentional (and easy to accidentally regress into a silent resolve later), let's pin it with a regression test that asserts both the count and the order:

it('same-tick breach-then-recover sends an alert followed by a resolved notification', async () => {
  // setup: threshold > 2, prior OK history at 22:05, now = 22:18
  //   buckets: [22:05-22:10] 3 errors (breach), [22:10-22:15] 1 error (ok)
  await processAlertAtTime(/* ... */);

  expect((await Alert.findById(details.alert.id))!.state).toBe('OK');
  expect(slack.postMessageToWebhook).toHaveBeenCalledTimes(2);
  // assert order: first call ALERT, second RESOLVED
});

Open to being talked out of it though - if you feel two back-to-back messages read as flapping and would rather collapse to a single ALERT that notes the value is already back to normal, I'm fine with that direction too. But absent a strong case, let's keep both + add the test.

@brandon-pereira agreed, I've added the regression test to pin the two notification behaviors, the test simulates a single evaluation tick processing back to back breach and recovery buckets, asserting that both the ALERT and RESOLVED notifications are emitted in the correct order and that the final persisted state correctly settles back to OK.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview, Comment	Jul 13, 2026 6:52pm
hyperdx-storybook	Ready	Preview, Comment	Jul 13, 2026 6:52pm

[brandon-pereira]

@Aryainguz can you please check the failing integration test when you get time? thank you!

[Aryainguz]

@Aryainguz can you please check the failing integration test when you get time? thank you!

@brandon-pereira The zero-fill test was asserting the wrong final state. Period 3 has 1 log and the alert fires when count < 1, so period 3 doesn't alert. Since buckets are evaluated oldest-to-newest, period 3 correctly auto-resolves period 2's alert to OK. Fixed the test to expect state: OK, counts: 0 instead of ALERT. I've tested locally as well all tests are successful please review once

[brandon-pereira]

LGTM - thanks for the contribution!