Skip to content

feat(alerts): fan out notifications to every configured channel - #2847

Open
jordan-simonovski wants to merge 6 commits into
jordansimonovski/alerts-multi-channel-apifrom
jordansimonovski/alerts-multi-channel-dispatch
Open

feat(alerts): fan out notifications to every configured channel#2847
jordan-simonovski wants to merge 6 commits into
jordansimonovski/alerts-multi-channel-apifrom
jordansimonovski/alerts-multi-channel-dispatch

Conversation

@jordan-simonovski

@jordan-simonovski jordan-simonovski commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Alert notifications now go to every configured channel concurrently, each with its own deadline, span and metrics. One slow or dead target can no longer delay the others or the alert evaluation loop.

What changed

Template rendering used to send inline: a Handlebars helper awaited each webhook as it rendered, so sends were serial and a hung endpoint blocked that alert indefinitely. Rendering now collects notification jobs, and dispatchNotifications runs them concurrently after the render.

Each send is wrapped in an alerts.notify CLIENT span, a per-target deadline, and per-target metrics, and never throws — the caller gets one result per target and records failures individually. Alert execution errors now name the webhook that failed, so a multi-channel alert says which target broke.

Each alert evaluation also gets a processAlert span carrying team context.

Key decisions

The deadline stops waiting; it does not cancel. Delivery stays at-least-once, so an abandoned send is allowed to finish. Its eventual rejection is swallowed so it cannot surface as an unhandled rejection and kill the task process.

A timed-out HTTP attempt is not retried. withRetry treats only 3xx and 4xx as terminal, and an abort surfaces as DOMException code 23 — so without this, a receiver that was merely slow would get three duplicate POSTs where it previously got one delivery. The timeout is surfaced as a 408 so the existing retry policy stops on it.

Per-attempt timeout defaults to 30s, not 10s. The bound exists to release a black-holed socket, not to police slow receivers; 30s leaves headroom inside the 60s deadline while not failing endpoints that succeed today.

renderAlertTemplate returns the rendered body alongside the results. Returning only the results would have made the rendering and template-injection assertions untestable, since those tests configure no webhooks and so produce no transport calls to inspect.

Impact

Behaviour changes for existing single-channel alerts, worth attention on merge:

  • Generic webhook attempts are now bounded by ALERT_NOTIFICATION_FETCH_TIMEOUT_MS (default 30s), and Slack sends by the same value. Neither had a per-attempt bound before.
  • A missing webhook no longer aborts the whole event; other channels still fire and the failure is recorded against that target.
  • Execution error messages now name the failing webhook.

New env vars: ALERT_NOTIFICATION_DEADLINE_MS (default 60s) and ALERT_NOTIFICATION_FETCH_TIMEOUT_MS (default 30s). Both fall back to the default when unset or malformed.

Implementation detail

MAX_NOTIFICATIONS_PER_EVENT (20) caps jobs per fire/resolve event, covering configured channels and @webhook- message mentions together. A channel dropped by the cap records an execution error rather than only a log line and a metric, so a partially-notified alert does not look healthy.

New metrics: hyperdx.alerts.notifications (attrs channel_type, service, outcome) and hyperdx.alerts.notification.duration_ms. The existing hyperdx.alerts.webhook_deliveries transport metrics are unchanged.

Tests cover failure isolation, deadline timeout for both generic and Slack targets, the abort-not-retried path, the malformed-env fallback, the per-event cap, and a pre-multi-channel document that only has channel.

Verification: 273 checkAlerts integration tests, plus the notification unit suite.

@changeset-bot

changeset-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: d94ce98

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@vercel

vercel Bot commented Aug 9, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Aug 20, 2026 2:02am
hyperdx-storybook Ready Ready Preview Aug 20, 2026 2:02am

Request Review

@greptile-apps

greptile-apps Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Alert rendering now collects notification jobs and concurrently dispatches every configured channel with isolated deadlines, tracing, metrics, and per-target error reporting.

  • Adds multi-channel fan-out with resolved-webhook deduplication and a per-event target cap.
  • Adds bounded generic and Slack webhook attempts and suppresses retries after ambiguous aborts or timeouts.
  • Adds integration and unit coverage for fan-out, failure isolation, deduplication, limits, and timeout behavior.

Confidence Score: 4/5

The PR is not yet safe to merge because failed message mentions can exhaust the notification cap and suppress healthy configured channels.

Unsupported or missing mentions are recorded before appended configured channels, and the cap counts those failures alongside actual jobs, allowing twenty invalid mentions to prevent every valid configured channel from being dispatched.

Files Needing Attention: packages/api/src/tasks/checkAlerts/template.ts

Important Files Changed

Filename Overview
packages/api/src/tasks/checkAlerts/template.ts Collects, resolves, deduplicates, caps, and concurrently dispatches notification jobs; the outstanding cap accounting still counts failed mentions as delivery slots.
packages/api/src/tasks/checkAlerts/index.ts Propagates multi-channel configuration and team context while recording target-specific notification failures.
packages/api/src/tasks/checkAlerts/transports/generic.ts Adds a fresh per-attempt abort timeout to generic webhook delivery.
packages/api/src/utils/retry.ts Treats abort and timeout errors as terminal to avoid ambiguous duplicate webhook delivery.
packages/api/src/utils/slack.ts Configures Slack webhook attempts with the shared notification timeout.
packages/api/src/tasks/checkAlerts/tests/multiChannelAlerts.int.test.ts Covers multi-channel delivery, target failure isolation, and legacy single-channel documents.

Sequence Diagram

sequenceDiagram
  participant Evaluator as processAlert
  participant Renderer as renderAlertTemplate
  participant Dispatcher as Notification Dispatcher
  participant A as Webhook A
  participant B as Webhook B
  Evaluator->>Renderer: Render alert and configured channels
  Renderer->>Renderer: Resolve, deduplicate, and cap targets
  Renderer->>Dispatcher: Dispatch jobs concurrently
  par Independent delivery
    Dispatcher->>A: Send with per-target deadline
  and Independent delivery
    Dispatcher->>B: Send with per-target deadline
  end
  Dispatcher-->>Renderer: Per-target outcomes
  Renderer-->>Evaluator: Rendered body and failures
Loading

Reviews (11): Last reviewed commit: "fix(alerts): dead barrel export, stale c..." | Re-trigger Greptile

Comment thread packages/api/src/tasks/checkAlerts/template.ts
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Scope: feat(alerts): fan out notifications to every configured channel — 14 files, ~1148 insertions, all under packages/api. Base 8c3d6090.

Intent (as reviewed against the code, not the description): renderAlertTemplate now collects notification jobs during Handlebars rendering and dispatches them concurrently via Promise.all after the render, isolating each target's failure; each webhook/Slack HTTP attempt is bounded by a per-attempt fetch timeout; timeouts/aborts are treated as non-retryable; a per-event cap limits fan-out; and each per-target failure is recorded as an alert execution error naming the target.

Note: the PR description references a per-target deadline (ALERT_NOTIFICATION_DEADLINE_MS), an alerts.notify CLIENT span, a processAlert span, and per-target metrics (hyperdx.alerts.notifications, …notification.duration_ms). None of these exist in the diff. This review covers the code as written; findings are not derived from the description.

✅ No critical issues found. No P0/P1 ship-blockers. Security review found no exploitable issues (SSRF guards, redirect rejection, template-injection invariant, and error-message redaction all hold). Recommendations below.

🟡 P2 — recommended

  • packages/api/src/tasks/checkAlerts/template.ts:498 — Inside the async NOTIFY helper, _hb.compile(idTemplate)(view) and _hb.compile(rawTemplateBody)(view) run outside the try/catch that only wraps getPopulatedChannel, so a malformed @webhook-{{…}} mention that fails Handlebars compilation rejects the entire render and drops every already-collected job for that fire/resolve event, defeating the per-target isolation this PR is built to provide.
    • Fix: Wrap the id/body compile-and-render calls in the same per-target guard and route a compile failure through recordPreFailure so one bad mention only fails its own target.
  • packages/api/src/utils/slack.ts:8getTimeoutMs duplicates the byte-identical env-parse logic of getWebhookFetchTimeoutMs in transports/generic.ts (same var, default, floor, NaN handling), so the two fetch-timeout paths can silently drift.
    • Fix: Extract one shared helper in a utils module and import it from both slack.ts and transports/generic.ts.
    • kieran-typescript
  • packages/api/src/utils/slack.ts:17 — The new Slack per-attempt timeout wiring (new IncomingWebhook(url, { timeout })) and its getTimeoutMs fallback have no test coverage: integration tests mock postMessageToWebhook, and no Slack unit test exists, so a regression dropping the option or breaking the env-var name would pass CI.
    • Fix: Add a slack.test.ts covering getTimeoutMs (unset/malformed → 30000, numeric → parsed) and asserting IncomingWebhook is constructed with { timeout }, mirroring transports/__tests__/generic.test.ts.
    • testing
🔵 P3 nitpicks (5)
  • packages/api/src/tasks/checkAlerts/template.ts:522 — The cap check jobs.length + failures.length >= MAX_NOTIFICATIONS_PER_EVENT counts unsupported @mention failures (recorded before the cap check) toward the limit, and user mentions render before appended configured channels, so a burst of junk mentions can exhaust the cap and starve real configured channels.
    • Fix: Count only dispatchable targets toward the cap, or record unsupported-mention failures without letting them consume the per-event budget for configured channels.
  • packages/api/src/tasks/checkAlerts/template.ts:498rawTemplateBody is recompiled via _hb.compile(rawTemplateBody)(view) once per target inside the NOTIFY helper, repeating identical compilation N times per event.
    • Fix: Compile the body template once before the fan-out and reuse the rendered result across targets.
  • packages/api/src/tasks/checkAlerts/__tests__/renderAlertTemplate.int.test.ts:972teamId is threaded into NotificationJob but no test asserts its value, and the production fireChannelEvent branch that derives teamId from a bare ObjectId is never exercised (all integration tests populate team).
    • Fix: Assert dispatched[0].teamId in the recording-dispatcher tests and add a case where alert.team is an unpopulated ObjectId.
    • testing
  • packages/api/src/tasks/checkAlerts/index.ts:1210makeNotificationAlertError's NotificationCapExceededError and UnsupportedMentionError branches lack end-to-end coverage through processAlert, so the exact stored executionError message format (including the (${target}) suffix) is unverified.
    • Fix: Add an integration case (>20 mentions, or an @here) that runs through processAlert and asserts the stored executionErrors[].message.
    • testing
  • packages/api/src/tasks/checkAlerts/index.ts:1229(details as any).dashboard / (details as any).savedSearch at the fireChannelEvent call site defeats the AlertDetails discriminated union; pre-existing, but now load-bearing for the multi-channel results contract.
    • Fix: Read dashboard/savedSearch from the correct union arm by switching on details.taskType instead of casting through any.
    • kieran-typescript

Reviewers (11 dispatched): correctness, reliability, adversarial, security, kieran-typescript, testing, maintainability, project-standards, performance, agent-native, learnings. Synthesis incorporates the completed returns (security, kieran-typescript, testing, learnings) plus orchestrator analysis of the full diff; the remaining reviewers had not returned at synthesis time, so behavior-level coverage (correctness/reliability/adversarial/performance) reflects orchestrator analysis rather than their independent verdicts.

Testing gaps:

  • No test asserts that WebhookResponseError's raw upstream response text never surfaces in the stored IAlertError.message (only the hardcoded fallback should).
  • No test covers a 3xx redirect propagating through the per-target path to confirm WEBHOOK_REDIRECT_ERROR_MESSAGE is recorded per target.
  • Concurrency of the Promise.all fan-out is not directly asserted (targets are attempted and isolated, but parallelism is not verified).
  • The generic timeout test asserts rejection but not that the error is specifically an abort/timeout.

@jordan-simonovski
jordan-simonovski force-pushed the jordansimonovski/alerts-multi-channel-dispatch branch from aab8217 to e092361 Compare August 10, 2026 01:32
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

🔴 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 — 466 lines (bar: 30):
    • packages/api/src/tasks/checkAlerts/errors.ts
    • packages/api/src/tasks/checkAlerts/index.ts
    • packages/api/src/tasks/checkAlerts/template.ts
    • packages/api/src/tasks/checkAlerts/transports/generic.ts
    • packages/api/src/tasks/checkAlerts/transports/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: 7
  • Production lines changed: 484 (+ 769 in test files, excluded from tier calculation)
  • Critical-path lines changed: 466
  • Branch: jordansimonovski/alerts-multi-channel-dispatch
  • Author: jordan-simonovski

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 github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Aug 10, 2026
Comment thread packages/api/src/tasks/checkAlerts/template.ts Outdated
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 305 passed • 1 skipped • 1038s

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

Tests ran across 4 shards in parallel.

View full report →

Comment thread packages/api/src/tasks/checkAlerts/template.ts Outdated
@jordan-simonovski
jordan-simonovski force-pushed the jordansimonovski/alerts-multi-channel-dispatch branch from a74e182 to c7ab03e Compare August 18, 2026 00:43
@jordan-simonovski
jordan-simonovski force-pushed the jordansimonovski/alerts-multi-channel-dispatch branch from c7ab03e to ab8397f Compare August 18, 2026 00:54
@jordan-simonovski
jordan-simonovski force-pushed the jordansimonovski/alerts-multi-channel-dispatch branch from ab8397f to 86ed25e Compare August 18, 2026 03:40
@jordan-simonovski
jordan-simonovski force-pushed the jordansimonovski/alerts-multi-channel-dispatch branch from 86ed25e to 748dbb1 Compare August 19, 2026 12:52
@jordan-simonovski
jordan-simonovski force-pushed the jordansimonovski/alerts-multi-channel-dispatch branch from 748dbb1 to 3b1e9e2 Compare August 19, 2026 23:46
Rebases the multi-channel dispatch work onto the dispatch-seam base
(NotificationDispatcher/NotificationJob in notifications.ts) instead of the
older dispatchNotifications/dispatchOne design, which is deleted entirely.

renderAlertTemplate resolves every configured channel plus @mention target,
dedupes by resolved webhook id before the MAX_NOTIFICATIONS_PER_EVENT cap
check, and hands one NotificationJob per surviving channel to the dispatcher.
Each dispatch is isolated so one failing channel can't block the others.

Pre-dispatch failures (unresolvable webhook, unsupported mention, cap
exceeded) still surface as execution errors. Actual delivery outcomes no
longer do: a queued dispatcher can't report them synchronously, so they're
metrics/logs in the transport layer instead. This is a deliberate behaviour
change from the pre-seam design, where a failed send always produced a
WEBHOOK_ERROR.
The inline dispatcher resolves after delivery, so a real send rejects and
reaches the caller — unlike a queued dispatcher, which resolves after enqueue
and can't report delivery synchronously. The per-job dispatch loop in
renderAlertTemplate was catching and swallowing that rejection, which meant a
webhook send failure stopped producing a WEBHOOK_ERROR execution error and
ERROR history row for every alert, not just the fan-out case.

Now the catch records the target and error into the same per-target failure
list pre-dispatch failures already use, so index.ts's existing
makeNotificationAlertError path picks it up unchanged. One failing channel
still can't stop the others — each dispatch keeps its own try/catch inside
the Promise.all.
…sables, fix teamId

- template.ts: an error handler that can itself throw is a defect. Add
  channelKey()/channelLabel() helpers that narrow on PopulatedAlertChannel's
  `type` discriminant instead of assuming `.channel` exists. Only the
  'webhook' variant exists in this repo today, but a downstream build adds
  more without a `channel` field — narrowing here makes that merge mechanical.
  Used at the dedupe-key site and inside the per-job dispatch catch block
  (both the log call and the failure record); the eventId computation is
  left untouched, it's pre-existing and out of scope.

- renderAlertTemplate.int.test.ts: removed two eslint-disable comments that
  should never have been added — the constraint was "no eslint-disable,
  don't spend the budget on suppression." Replaced the two `as unknown as
  IWebhook` fixture casts with a single shared `castWebhook` helper (same
  single-narrowing-point pattern as `partialAlert` in checkAlerts.int.test.ts),
  so the unsafe assertion exists once in source instead of at every call site.

- index.ts: alert.team is typed as a bare ObjectId, but int-test setups
  populate it into a full Team document (the production path never does).
  Mongoose documents don't override toString(), so that silently produced
  "[object Object]" instead of the hex id, feeding setBusinessContext with
  garbage. Added a type-guard (no `as` assertion needed) that prefers the
  populated document's own _id when present.
…ep check

The comment mentioned "eslint-disable" as prose, which false-positives the
verification command (git diff ... | grep '^+.*eslint-disable'). No code
change.
The generic webhook transport called fetch() with no signal, so a
receiver that accepts the connection and never responds hung the send
indefinitely, and withRetry could compound it across attempts.
getWebhookFetchTimeoutMs() existed for this but had no call site.

Wire it in as AbortSignal.timeout(), created fresh inside the
withRetry callback so it bounds one attempt instead of the whole
retry sequence. Combine it with a caller-supplied signal via
AbortSignal.any() when one is passed through ChannelTransport's ctx.

Treat an abort/timeout as non-retryable in withRetry, matching the
existing intent in utils/slack.ts: retrying an ambiguous timeout can
duplicate delivery on a receiver with no idempotency guarantee.

Remove the knip @public tag on getWebhookFetchTimeoutMs now that it
has a real caller, and add unit coverage (in transports/__tests__ and
utils/__tests__/retry.test.ts) proving a hanging receiver is actually
aborted and that the abort isn't retried.
Four small fixes found while auditing the multi-channel dispatch path:

- Delete the transports barrel's getWebhookFetchTimeoutMs re-export. Its
  @public tag was hiding real dead code: every caller imports it from
  ./generic directly, and nothing imports it from the barrel.

- Reword the AbortSignal.any() doc comment on getWebhookFetchTimeoutMs.
  deliverNotification only ever passes { group }, so ctx.signal is always
  undefined and that branch never runs today. The signal parameter stays
  for a future queued dispatcher that needs to cancel in-flight deliveries.

- Document that getDefaultExternalActions' @mention round-trip is lossy:
  only type and webhookId survive it, so anything needing other
  per-channel fields at delivery time has to be threaded separately, and
  reading alert.channel to recover them gets channels[0]'s value for
  every channel. Not fixing the round-trip itself here.

- Thread the channel type through NotificationFailure so
  makeNotificationAlertError can report it instead of hardcoding
  "webhook", which misdescribes a non-webhook channel's failure on a
  fork that adds one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant