feat(alerts): fan out notifications to every configured channel - #2847
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryAlert rendering now collects notification jobs and concurrently dispatches every configured channel with isolated deadlines, tracing, metrics, and per-target error reporting.
Confidence Score: 4/5The 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
|
| 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
Reviews (11): Last reviewed commit: "fix(alerts): dead barrel export, stale c..." | Re-trigger Greptile
Deep ReviewScope: Intent (as reviewed against the code, not 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
🔵 P3 nitpicks (5)
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:
|
aab8217 to
e092361
Compare
🔴 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
|
E2E Test Results✅ All tests passed • 305 passed • 1 skipped • 1038s
Tests ran across 4 shards in parallel. |
cf3d49d to
57557be
Compare
57557be to
a74e182
Compare
a74e182 to
c7ab03e
Compare
c7ab03e to
ab8397f
Compare
ab8397f to
86ed25e
Compare
86ed25e to
748dbb1
Compare
748dbb1 to
3b1e9e2
Compare
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.
3b1e9e2 to
d94ce98
Compare
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.