[wrn14897]

Summary

On a deployment with about 2,900 alerts, one check-alerts tick took about 3 minutes, with the worker's Node thread at 100% CPU. One-minute alerts were evaluated every 3 to 4 minutes, so every notification was late.

The default provider hands every alert in a tick the same previousMap. That map holds the latest history of every group of every alert. Each alert then scans it in full: once in shouldSkipAlertCheck (about 21 ms per alert on that deployment), and again in getAlertEvaluationDateRange, the empty-bucket check and the missing-group loop. Tick CPU grew with alerts times total groups.

This PR adds partitionPreviousMapsByAlert and calls it once in CheckAlertTask.execute(), right after getAlertTasks():

• It walks each distinct map once and gives every alert a map that holds only its own entries (key alertId or alertId||group). Alerts with no history get an empty map.
• Keys and values are unchanged, so no reader changes. recentHistoryMap is left alone because it is only read with .get().
• It runs in the task, not the loader, so it covers every AlertProvider.
• The input is not mutated.

Side effect: on main, the empty-bucket check (previousMap.values().some(...)) and the missing-group loop read entries of other alerts. As far as I can tell from the code (not reproduced), a grouped alert could write a spurious OK history for group '' when a different alert was ALERT or PENDING. With per-alert maps that no longer happens on the task path. Callers that pass a shared map straight to processAlert behave as before.

Not in this PR: reading less history from the database (#3508 and #3509).

How to reproduce

Starting point: a local checkout. The change is in the check-alerts worker and has no UI.

On main:

1. Add the should pass each alert only its own previous history test from this PR, with its buildHistory and buildPartitionTask helpers, to packages/api/src/tasks/checkAlerts/__tests__/checkAlertsTask.test.ts.
2. From packages/api, run yarn ci:unit src/tasks/checkAlerts/__tests__/checkAlertsTask.test.ts. It fails: both alerts receive all three history entries.

On this branch:

1. Run the same command. All 13 tests pass, and each alert receives only its own entries.
2. make dev-int FILE="checkAlerts.int.test --testNamePattern=shared.by.every.alert" runs two integration tests that split a shared map and evaluate a grouped alert for real while another alert is ALERT. Both fail if the map reaches processAlert unsplit: one gets an extra empty-group OK history, the other records 1 of its 3 empty buckets.
3. make dev-int FILE=checkAlerts passes.

How to test on Vercel preview

N/A — non-UI change.

References

• Linear Issue: https://linear.app/clickhouse/issue/HDX-6022/check-alerts-tick-is-cpu-bound-every-alert-scans-the-shared-group
• Related PRs: fix(api): read previous alert history with one index seek per group #3509. Related issue: Grouped alerts: getPreviousAlertHistories reads every history row of the last 7 days, so check-alerts slows down and runs out of memory as history grows #3508 (history read cost, not fixed here)

[vercel]

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

Project	Deployment	Actions	Updated
hyperdx-oss	Ready	Preview	Nov 14, 2026 6:31am UTC
hyperdx-storybook	Ready	Preview	Nov 14, 2026 6:31am UTC

[changeset-bot]

🦋 Changeset detected

Latest commit: f4204fd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@hyperdx/api	Patch
@hyperdx/app	Patch
@hyperdx/otel-collector	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

Duplicate code analysis

No new duplicate code introduced by this PR.

---

jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 108 in total (1.18%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

[github-actions]

🔴 Tier 4 — Critical

Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI.

Why this tier:

• Background tasks or delivery pipeline substantially modified — 46 lines (bar: 30):
  • packages/api/src/tasks/checkAlerts/index.ts

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats

• Production files changed: 1
• Production lines changed: 46 (+ 310 in test files, excluded from tier calculation)
• Branch: warren/alert-history-query-perf-HDX-6022
• Author: wrn14897

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[github-actions]

🔵 minor — Per-alert history is already split at the source, then merged into one map and split again by parsing keys

getPreviousAlertHistories (index.ts:1983-2045) already runs one query per alert and gets results[i] for alertIds[i]. It then flattens them into one shared map, and this helper rebuilds the per-alert split by running indexOf('||') on each key. Two ways to fix it: have the source return Map<alertId, Map<key, history>> and let loadAlert (providers/default.ts:403-407) attach previousAlerts.get(alert.id). Or, if the task-level pass stays so it covers every provider, bucket by history._id.toString() instead of parsing the key. Then the split doesn't depend on the delimiter.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[github-actions]

🔵 minor — No test covers the behaviour change on the task path: other alerts' ALERT/PENDING rows no longer affect a grouped alert

The PR changes what processAlert does in two places. hasAlertsInPreviousMap (index.ts:1708) now only sees the alert's own rows, and so does the missing-group loop (index.ts:1811-1833), which used to map another alert's bare key to group '' through extractGroupKeyFromMapKey. The new tests only check which map processAlert receives (it's mocked), so neither change is tested. Add an integration case: a grouped alert with an empty bucket, while a different alert's history is ALERT. Assert that no spurious '' OK history is written and that the empty-bucket branch takes the !hasAlertsInPreviousMap path.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[github-actions]

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

• 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
• 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
• Anything about surrounding code, or asking you to widen the change (hoist a helper,
  dedupe with another file, fix other call sites): never fix here, whatever the
  severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor

1 posted as inline comment(s) on the changed lines.

---

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

[github-actions]

E2E Test Results

✅ All tests passed • 465 passed • 1 skipped • 1001s

Status	Count
✅ Passed	465
❌ Failed	0
⚠️ Flaky	3
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

Deep Review

✅ No critical issues found.

The change is small, well-scoped, and strongly tested. partitionPreviousMapsByAlert is behavior-preserving for every reader (keys and values are unchanged; readers already filter by the alertId/alertId||group key shape), it builds buckets once per distinct shared-map object and then does O(1) per-alert lookups, and it does not mutate the input tasks or maps. The unit tests cover per-alert split, unchanged keys, a group name containing the || delimiter, maps shared across tasks vs. already-separate maps, no-mutation, the empty-map case for an alert with no history, and the execute() wiring; two integration tests exercise the real grouped-alert path beside a firing alert and would fail against an unsplit map.

No P2 items rise above advisory for this diff.

🔵 P3 nitpicks (2)

• packages/api/src/tasks/checkAlerts/index.ts:648 — The bucket builder reverse-derives the owning alert id with key.indexOf(ALERT_GROUP_DELIMITER) / substring, a second key-parsing scheme alongside the canonical getAlertKeyPrefix / computeHistoryMapKey / extractGroupKeyFromMapKey helpers; correctness relies on the same "alert ids are hex ObjectIds, so || never appears in an id" assumption those helpers document, but that assumption is not restated here.
  • Fix: Add a short comment at the extraction site noting it depends on the ObjectId-is-hex delimiter guarantee, so the two parsing paths are kept in sync intentionally.
• packages/api/src/tasks/checkAlerts/index.ts:2266 — The split runs once in execute() before the per-task error boundary in processAlertTask, so any throw while iterating a task's previousMap would fail the whole tick rather than the single task; the trigger requires a provider to violate the non-optional Map contract, so the risk is defensive only.
  • Fix: Treat a missing or non-iterable previousMap as an empty map in partitionPreviousMapsByAlert, or perform the split inside the existing per-task try/catch.

---

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

Testing gaps:

• Only the task path is partitioned; direct callers that pass an unsplit shared map to processAlert retain the prior empty-group behavior and have no regression test — acceptable as scoped, but worth a one-line note in the code or test.
• No explicit case for an alert id that itself contains || (currently impossible for hex ObjectIds); fine while that invariant holds, but it is the single assumption the split depends on.

Prior PR comments (5) were all automated (Vercel, changeset-bot, jscpd, pr-triage, e2e-results) with no human review feedback to verify, so the previous-comments reviewer was not run.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

[github-actions]

🔵 minor — Cross-alert history leak is fixed in a wrapper, not in processAlert where the bug is

Move the per-alert scoping to the layer that causes the problem, so every path gets it and the key isn't parsed in a second place. Two options. (1) In getPreviousAlertHistories (index.ts:1983-2045), results already arrives as one array per alert. Return a Map<alertId, Map<key, history>> from there and have loadAlert (providers/default.ts:403-406) attach previousMap: byAlert.get(alert.id) ?? new Map(). That avoids re-deriving the alert id with a second hand-written indexOf('||') split alongside getAlertKeyPrefix/extractGroupKeyFromMapKey. (2) Apply the same key === alert.id || key.startsWith(getAlertKeyPrefix(alert.id)) filter that shouldSkipAlertCheck already uses to the hasAlertsInPreviousMap check (index.ts:1708) and to the missing-group loop (index.ts:1811-1834). As written, processAlert still writes a spurious group-'' OK history whenever it gets a shared map, and the PR itself says direct callers 'behave as before'.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}