feat(alerts): fan out notifications to every configured channel - #2847
Merged
jordan-simonovski merged 6 commits intoAug 21, 2026
Merged
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This was referenced Aug 9, 2026
Contributor
Greptile SummaryThe PR changes alert rendering to collect notification jobs and fan them out concurrently with per-target deadlines, tracing, metrics, and isolated error reporting.
Confidence Score: 4/5The PR is not yet safe to merge because failed message mentions can still exhaust the notification allowance and prevent valid configured channels from receiving an alert. Unsupported or missing mentions are processed before configured channels and added to Files Needing Attention: packages/api/src/tasks/checkAlerts/template.ts
|
| Filename | Overview |
|---|---|
| packages/api/src/tasks/checkAlerts/template.ts | Collects, resolves, deduplicates, caps, and dispatches notification jobs, but unresolved mentions still incorrectly consume valid delivery slots. |
| packages/api/src/tasks/checkAlerts/index.ts | Integrates per-target notification results into alert execution errors and adds per-alert tracing context. |
| packages/api/src/tasks/checkAlerts/transports/generic.ts | Adds bounded generic-webhook attempts while preserving retry and delivery metrics behavior. |
| packages/api/src/utils/slack.ts | Adds bounded Slack webhook attempts using the shared notification timeout configuration. |
| packages/api/src/tasks/checkAlerts/tests/renderAlertTemplate.int.test.ts | Covers resolved-ID deduplication and ordinary cap behavior, but not failed mentions consuming the cap. |
| packages/api/src/tasks/checkAlerts/tests/multiChannelAlerts.int.test.ts | Exercises multi-channel fan-out, failure isolation, single-channel failure recording, and legacy alert compatibility. |
Sequence Diagram
sequenceDiagram
participant Eval as processAlert
participant Render as renderAlertTemplate
participant Dispatch as Notification Dispatcher
participant Targets as Webhook Targets
Eval->>Render: Render alert and collect targets
Render->>Render: Resolve and deduplicate webhooks
Render->>Dispatch: Submit notification jobs concurrently
par Per-target delivery
Dispatch->>Targets: Send with attempt timeout
and Independent delivery
Dispatch->>Targets: Send with attempt timeout
end
Dispatch-->>Render: Per-target outcomes
Render-->>Eval: Rendered body and failures
Eval->>Eval: Record target-specific execution errors
Reviews (15): Last reviewed commit: "fix(alerts): dead barrel export, stale c..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Deep ReviewScope: 14 files vs base ✅ No critical (P0/P1) issues found. The concurrency, dedup, teamId narrowing, non-retry-on-abort, and Promise.all isolation paths were each independently traced and hold up. The items below are recommended improvements. 🟡 P2 -- recommended
🔵 P3 nitpicks (8)
Reviewers (12): correctness, security, adversarial, reliability, performance, api-contract, kieran-typescript, testing, maintainability, project-standards, agent-native, learnings-researcher. Testing gaps:
|
jordan-simonovski
force-pushed
the
jordansimonovski/alerts-multi-channel-dispatch
branch
from
August 10, 2026 01:32
aab8217 to
e092361
Compare
Contributor
🔴 Tier 4 — CriticalTouches 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:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
github-actions
Bot
added
the
review/tier-4
Contributor
E2E Test Results✅ All tests passed • 307 passed • 1 skipped • 1169s
Tests ran across 4 shards in parallel. |
jordan-simonovski
force-pushed
the
jordansimonovski/alerts-multi-channel-dispatch
branch
from
August 17, 2026 02:58
cf3d49d to
57557be
Compare
jordan-simonovski
force-pushed
the
jordansimonovski/alerts-multi-channel-dispatch
branch
from
August 17, 2026 05:46
57557be to
a74e182
Compare
pulpdrew
previously approved these changes
Aug 20, 2026
pulpdrew
left a comment
pulpdrew
left a comment
Contributor
There was a problem hiding this comment.
LGTM, couple of tiny naming / type ideas but nothing blocking
| /** The webhook id/name prefix, or the raw @mention, that failed. */ | ||
| target: string; | ||
| /** The channel type the target belongs to, or 'unknown' when it couldn't be determined (e.g. an unparseable @mention). */ | ||
| type: string; |
Contributor
There was a problem hiding this comment.
Suggested change
| type: string; | |
| type: AlertChannelType | 'unknown'; |
| message: `${WEBHOOK_REDIRECT_ERROR_MESSAGE} (${target})`.slice(0, 10000), | ||
| }; | ||
| } | ||
| // A delivery rejection from the inline dispatcher — the only case left. |
Contributor
There was a problem hiding this comment.
Is this something we can typecheck or assert? It would be nice if the type system told us when we forgot to handle a new error case here.
Contributor
Author
There was a problem hiding this comment.
Good question, and I dug into it — the answer is "not cleanly, and here's why".
The final branch is a genuine catch-all for a dispatcher rejection, and NotificationFailure.error is typed unknown because anything can be thrown through it. Exhaustiveness checking needs a discriminant to switch on; instanceof narrowing on unknown doesn't produce one, so there's no never for the compiler to complain about. Adding a default that assigns to never today would just fail to compile against unknown.
Doing it properly means introducing a wrapper error type at the dispatch boundary so every failure arrives carrying a discriminant, then switching on that. That's a real improvement and I think worth doing — but it also touches the parallel failure paths downstream, so I've left it rather than half-applying it. A partially-applied exhaustiveness check is worse than none: it looks like it's protecting you and isn't.
Your other three are in #2961, along with a P1 Greptile raised here that didn't make it in before this merged (failed @mentions were consuming notification cap slots, so a real webhook could get silently skipped and reported as cap-exceeded).
| // cleaning this up next. fireChannelEvent guards against null values | ||
| // for these properties. | ||
| await fireChannelEvent({ | ||
| const results = await fireChannelEvent({ |
Contributor
There was a problem hiding this comment.
Suggested change
| const results = await fireChannelEvent({ | |
| const failures = await fireChannelEvent({ |
| /** The rendered message body, as delivered to every target. */ | ||
| body: string; | ||
| /** One entry per target that did not end up delivered — see NotificationFailure. */ | ||
| results: NotificationFailure[]; |
Contributor
There was a problem hiding this comment.
Suggested change
| results: NotificationFailure[]; | |
| failures: NotificationFailure[]; |
| // ordinary mention like "@here" arrives with an unsupported channel type. | ||
| // Parsing inside the guard keeps that from rejecting the whole render and | ||
| // dropping every already-collected job. | ||
| const parsed = zNotifyFnParams.safeParse(options); |
Contributor
There was a problem hiding this comment.
Aside, I know it was pre-existing, but this whole handlebar templating / @ rewrites seems so unnecessarily complicated. I gather it was added to support conditional routing on attributes based on the user's webhook template, and it's kind of a hack to support multiple webhooks prior to these changes? It just seems odd to (a) drive the alert-level channels through a channel --> @ --> helper --> channel rewrite and (b) destroy any non-channel-related @ mention in the template in the process.
Contributor
Author
There was a problem hiding this comment.
Yeah that's fair. I'll look into tackling this next
jordan-simonovski
dismissed
pulpdrew’s stale review
August 20, 2026 21:30
The merge-base changed after approval.
jordan-simonovski
force-pushed
the
jordansimonovski/alerts-multi-channel-dispatch
branch
from
August 20, 2026 21:31
d94ce98 to
aef90e6
Compare
pulpdrew
approved these changes
Aug 20, 2026
jordan-simonovski
force-pushed
the
jordansimonovski/alerts-multi-channel-dispatch
branch
from
August 21, 2026 01:23
aef90e6 to
6416532
Compare
jordan-simonovski
force-pushed
the
jordansimonovski/alerts-multi-channel-dispatch
branch
from
August 21, 2026 01:44
6416532 to
7acb371
Compare
Base automatically changed from
jordansimonovski/alerts-multi-channel-api
to
main
August 21, 2026 02:32
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.
jordan-simonovski
force-pushed
the
jordansimonovski/alerts-multi-channel-dispatch
branch
from
August 21, 2026 02:32
7acb371 to
c98d6ed
Compare
kodiakhq Bot
pushed a commit
that referenced
this pull request
Aug 21, 2026
) Follow-ups from review on #2847, which merged before these were applied. ## The bug (Greptile P1, raised twice on #2847) The per-event notification cap counted pre-dispatch *failures* toward the limit: ```ts if (jobs.length + failures.length >= MAX_NOTIFICATIONS_PER_EVENT) { ``` `MAX_NOTIFICATIONS_PER_EVENT` is 20. An ordinary `@here` in an alert message body arrives as an unsupported channel type and becomes a failure — so it burns one of the 20 slots despite delivering nothing. With enough unresolvable mentions ahead of them, **configured webhooks get silently skipped and reported as cap-exceeded**: a missed notification *and* a misleading error explaining it as something else. The cap exists to bound how many notifications one event sends. A failure sends none, so it shouldn't consume budget. Now counts jobs only. This is live on `main` as of #2847. ### Test `renderAlertTemplate.int.test.ts` gains a case putting several unresolvable mentions ahead of a configured webhook and asserting the webhook still gets a job. Verified it fails against the old counting before being fixed — restoring `jobs.length + failures.length` turns it red. ## Reviewer follow-ups from @pulpdrew - **`NotificationFailure.type`** tightened from `string` to `AlertChannelType | 'unknown'`. `'unknown'` is what the unparseable-mention path already used; the doc comment said so but the type didn't enforce it. - **`const results = await fireChannelEvent(...)` → `failures`** in `index.ts`. The array only ever holds failures. The `RenderedAlert.failures` half of this rename landed with #2847; the call site didn't. ## On the exhaustiveness question @pulpdrew asked whether the error-type mapping could be typechecked so the compiler catches an unhandled case. Not cleanly, and not in this PR. The final branch is a genuine catch-all for a dispatcher rejection, and `NotificationFailure.error` is `unknown` — anything can be thrown. `instanceof` narrowing on `unknown` can't produce a `never` check, so there's no discriminant to exhaust. Doing it properly means introducing a wrapper error type at the dispatch boundary so every failure carries a discriminant, which also touches the parallel failure paths downstream. Worth doing, but as its own change rather than half-applied here. ## Verification `renderAlertTemplate` 79/79, `checkAlerts` 294/294, api lint 302/302 with 0 errors, `tsc` clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
dispatchNotificationsruns them concurrently after the render.Each send is wrapped in an
alerts.notifyCLIENT 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
processAlertspan 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.
withRetrytreats only 3xx and 4xx as terminal, and an abort surfaces asDOMExceptioncode 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.
renderAlertTemplatereturns 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:
ALERT_NOTIFICATION_FETCH_TIMEOUT_MS(default 30s), and Slack sends by the same value. Neither had a per-attempt bound before.New env vars:
ALERT_NOTIFICATION_DEADLINE_MS(default 60s) andALERT_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(attrschannel_type,service,outcome) andhyperdx.alerts.notification.duration_ms. The existinghyperdx.alerts.webhook_deliveriestransport 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.