[wrn14897]

Summary

When an alert recovers, the check-alerts task sends one resolve notification. If that webhook delivery failed, the next evaluation saw the alert as OK and had nothing to resolve. The incident stayed open in the receiving tool (incident.io, PagerDuty via webhook, etc.) until someone closed it by hand. Firing notifications don't have this problem, because ALERT is sent again on every evaluation.

This PR sends a failed resolve again until it is delivered:

• Stored durably. A resolve whose delivery rejects is saved to a new PendingNotification collection, one record per alert, channel and group (keyed by eventId). Failures before dispatch (deleted webhook, notification cap, unknown mention) are not stored, since sending them again cannot succeed. Failed ALERT notifications are not stored either.
• Re-sent each tick, ahead of evaluation. CheckAlertTask loads the pending records for all alerts in one query. Before each alert is evaluated, its records are sent again with the stored message and the same Idempotency-Key. Running first means a resolve reaches the receiver before any new ALERT. Retries back off from 1 to 30 minutes, independent of the alert's interval, so a 1d alert does not wait a day between tries.
• Stopping rules. A record is dropped when the group fires again (a late resolve would close the new incident) or its webhook is deleted. It is held while the alert is silenced. After 24 hours it is dropped and a WEBHOOK_ERROR is appended to the ERROR history row of the window that resolved, so it shows in the alert's evaluation history.
• Better failure logs. serialize-error drops cause, which is where undici puts the real reason behind fetch failed. A new serializeErrorWithCause keeps the cause chain and AggregateError.errors. It is used in the webhook transport, the dispatch-failure logs and withRetry.
• Observability. New counter hyperdx.alerts.notification_resends with outcome (delivered, failed, expired, superseded, webhook_missing, skipped_silenced), plus logs for each attempt and for a resolve that was given up on.

The re-send loop goes through new AlertProvider methods, like the other state writes. The default provider implements them on Mongo. The model lives under models/ and is reached from the API process through the same import chain as the providers, so Mongoose builds its indexes there; the worker runs with autoIndex: false.

Not in this PR:

• The downstream in-process delivery queue resolves dispatch() before delivery, so it never reports a failure here. It needs a separate change to store failed resolves from the queue.
• Re-sending failed ALERT notifications. That only matters if notify-once ships for firing alerts.
• A UI for delivery history or redelivery.

How to reproduce

Starting point: full stack with yarn dev. This needs the API and the alert task.

On main:

1. Create a generic webhook whose URL points at an endpoint you can turn on and off, such as a local nc -l or a request bin you can disable.
2. Create a 1m saved-search alert on that webhook with threshold "above 0" on a query that currently matches logs.
3. Wait for the ALERT notification to arrive.
4. Make the endpoint unreachable, then make the query stop matching (change the alert's threshold or stop the log source).
5. When the alert goes OK the resolve fails, and the alert's evaluation history shows a webhook error.
6. Bring the endpoint back. No resolve ever arrives.

On this branch:

1. Repeat steps 1 to 5.
2. Bring the endpoint back. The resolve arrives on a following tick (within about 1, 2, 4, … minutes of the failure), with the same Idempotency-Key header as the failed attempt.
3. The pendingnotifications Mongo collection is empty again.

Testing

• pendingNotifications.int.test.ts (new) covers:
  • storing a failed resolve;
  • delivery on a later tick with the same body and Idempotency-Key;
  • backoff after another failure;
  • storing only the channel that failed;
  • not storing failed ALERTs;
  • dropping on re-fire;
  • recording the error and stopping after 24h;
  • dropping when the webhook is deleted;
  • holding while silenced.
• checkAlertsTask.test.ts checks that pending records are loaded once and re-sent before processAlert, and that a failed load does not stop evaluation.
• Unit tests cover backoff and serializeErrorWithCause.
• The existing test for a failed resolve now also checks that a record is stored.
• Ran make ci-lint, make ci-unit and make dev-int FILE=checkAlerts/; all pass.

References

• Linear Issue: HDX-6020
• Related PRs:

[changeset-bot]

🦋 Changeset detected

Latest commit: aff42fe

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

[vercel]

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

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

[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]

E2E Test Results

✅ All tests passed • 467 passed • 1 skipped • 983s

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

Tests ran across 4 shards in parallel.

View full report →

[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 — 416 lines (bar: 30):
  • packages/api/src/tasks/checkAlerts/index.ts
  • packages/api/src/tasks/checkAlerts/pendingNotifications.ts
  • packages/api/src/tasks/checkAlerts/providers/default.ts
  • packages/api/src/tasks/checkAlerts/providers/index.ts
  • packages/api/src/tasks/checkAlerts/template.ts
  • packages/api/src/tasks/checkAlerts/transports/generic.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: 9
• Production lines changed: 507 (+ 493 in test files, excluded from tier calculation)
• Critical-path lines changed: 416
• Branch: warren/HDX-6020-resend-failed-resolve
• 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 — Resends call deliverNotification directly and skip the NotificationDispatcher

Resends go straight to deliverNotification, but notifications.ts says a NotificationDispatcher owns delivery. fireChannelEvent also accepts a dispatcher (template.ts:623). So the downstream queueing dispatcher the PR description mentions would be skipped for resends, and so would any dedup or rate limiting it adds. Fix: route resends through the same dispatcher used for first sends, for example by passing it into resendPendingNotifications.

_{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.

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

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

Findings outside the changed lines

1 minor

• 🔵 packages/api/src/tasks/checkAlerts/__tests__/pendingNotifications.int.test.ts:371 — Integration test rebuilds getPreviousState itself, so the grouped-alert superseded path is never tested → The resend helper writes its own getPreviousState as previousMap.get(alert.id), and every test uses an alert without groups. The production closure in processAlertTask (index.ts, computeHistoryMapKey(alert.alert.id, group)) never runs here. A wrong group key there would send a late resolve that closes a re-fired incident, and these tests would still pass. Fix: add a group-by alert case that runs through CheckAlertTask.processAlertTask, or export the key lookup and use it in the helper.

---

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

[github-actions]

Deep Review

✅ No critical issues found. No P0/P1 defects surfaced: the resend path is wrapped so a single bad record cannot abort evaluation (resendPendingNotifications catches per-record and getPendingNotificationsByAlert swallows load errors), the Idempotency-Key is faithfully reproduced because the stored message is replayed verbatim, previousMap is populated before the resend runs, and the 24h appendAlertErrorHistory push targets the ERROR history row that the original dispatch failure already created for that window. The items below are recommendations and nits.

🟡 P2 — recommended

• packages/api/src/models/pendingNotification.ts:50 — the unique eventId index (the integrity guarantee behind savePendingNotifications' replaceOne+upsert) is only built if the model is registered in a process with autoIndex enabled; the worker runs connectDB({ autoIndex: false }) and the API process reaches the model only through a type-position import chain (template.ts imports AlertProvider/PopulatedAlertChannel as types), which tsc can elide under isolatedModules/NodeNext, leaving the unique index unbuilt and concurrent upserts able to create duplicate records (duplicate resolve resends).
  • Fix: guarantee the model is value-imported during API startup (e.g. register it in the models barrel) or call syncIndexes() for it, and add a test asserting the unique eventId index exists.

🔵 P3 nitpicks (2)

• packages/api/src/tasks/checkAlerts/pendingNotifications.ts:132 — const message: Message = JSON.parse(record.message) casts untyped parsed data straight to Message before delivering it to an external webhook, so a corrupted or schema-drifted stored record ships a bad payload or throws unhandled rather than failing loudly.
  • Fix: validate the parsed value against the Message schema (or a narrowing guard) before use.
• packages/api/src/utils/errors.ts:77 — the internal recursion counter is exposed as a public second parameter (serializeErrorWithCause(error, depth = 0)), letting a caller pass a non-zero depth and silently truncate the cause chain.
  • Fix: move the recursion into a private inner helper and keep the exported function single-argument.

---

Reviewers (10): correctness, reliability, adversarial, kieran-typescript, testing, maintainability, performance, api-contract, data-migrations, project-standards.

Testing gaps:

• No test exercises serializeErrorWithCause against a self-referential cause cycle hitting MAX_CAUSE_DEPTH, nor the AggregateError.errors fan-out with nested causes.
• No test covers resendOne when record.message is malformed/legacy JSON (the unchecked-cast path).
• Residual: savePendingNotifications' replaceOne resets attempts/firstFailedAt; confirm no cross-tick sequence can replace a live record and extend the 24h resend window.

_{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}