[HDX-5037] Deliver alert notifications on a cross-tick queue with delivery-failure feedback - #2915
[HDX-5037] Deliver alert notifications on a cross-tick queue with delivery-failure feedback#2915wrn14897 wants to merge 1 commit into
Conversation
…with delivery-failure feedback Notification deliveries were awaited inline inside processAlert, so each send occupied an alert-evaluation slot for its full duration (including retries against slow or dead endpoints), extending the tick and risking skipped cron fires. Evaluations now dispatch fully rendered NotificationJobs to an InProcessNotificationDispatcher and continue immediately: - NotificationJob: versioned, fully serializable core (zod) designed as a future notification service's wire contract — eventId (the existing alert+channel+group objectHash) is the idempotency key, channels are referenced by id. The resolved channel rides along as a transport-local sidecar for in-process delivery. - NotificationDispatcher interface (dispatch/shutdown/ drainDeliveryFailures), mirroring the AlertProvider pattern; InlineNotificationDispatcher is the default, so direct processAlert calls without a dispatcher keep today's inline delivery. - Bounded worker pool (concurrency 20) delivers in the background; per-eventId FIFO chaining guarantees a RESOLVED notification never overtakes its still-pending ALERT for the same alert+channel+group. - Bounded depth (5k); overflow drops with a log line and the hyperdx.alerts.notifications_dropped counter. Delivery health is on the alerts.notification_delivery operation SLI. - Process-lifetime singleton: CheckAlertTask instances are per cron tick, but deliveries outlive the tick that produced them. The one-shot runner path (RUN_SCHEDULED_TASKS_EXTERNALLY) drains with a 60s deadline before exiting. Because alert state now persists before the webhook lands, a naive queue would silently drop delivery errors from the alert's executionErrors — the alert details page would show a permanently failing webhook as healthy. Instead, the dispatcher buffers each delivery failure per alert (bounded: 5 per alert, 10k alerts) and the alert's next evaluation drains them into its executionErrors via drainDeliveryFailures(), so webhook failures still surface on the alert (one tick late, clearing once deliveries succeed) and as ERROR rows in the evaluation history. Accepted semantics vs inline delivery: a crash loses queued notifications instead of duplicating them, and failure visibility lags by one tick. Also moves makeAlertError/makeWebhookAlertError/getErrorMessage from checkAlerts/index.ts into checkAlerts/errors.ts so the queue can build IAlertErrors without an import cycle.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🔴 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
Knip - Unused Code Analysis🔴 1 issue found Unused exports (1)
Knip finds unused files, dependencies, and exports in your codebase. |
Greptile SummaryThe PR moves alert notification delivery onto a bounded, process-wide worker queue and feeds asynchronous delivery failures back into subsequent alert evaluations.
Confidence Score: 4/5The external-scheduler path should not be merged until queued delivery failures survive across one-shot process invocations; the queue module should also be split to meet repository structure requirements. In one-shot mode, webhook failures are buffered only in memory and discarded when the runner exits, preventing the promised executionErrors and ERROR-history feedback from ever reaching the next evaluation. Files Needing Attention: packages/api/src/tasks/index.ts and packages/api/src/tasks/checkAlerts/notificationQueue.ts
|
| Filename | Overview |
|---|---|
| packages/api/src/tasks/checkAlerts/notificationQueue.ts | Adds the bounded cross-tick queue, FIFO chains, telemetry, and failure buffer; its process-local feedback cannot survive external one-shot execution and the file exceeds the repository size limit. |
| packages/api/src/tasks/checkAlerts/index.ts | Routes alert notifications through the singleton dispatcher and drains buffered failures into evaluation persistence. |
| packages/api/src/tasks/checkAlerts/notifications.ts | Defines the versioned serializable job contract and dispatcher interface with inline-delivery compatibility. |
| packages/api/src/tasks/checkAlerts/template.ts | Separates delivery from rendering and constructs fully rendered notification jobs for dispatch. |
| packages/api/src/tasks/index.ts | Adds bounded queue shutdown before one-shot process exit, but exits without preserving buffered delivery failures for the next invocation. |
Sequence Diagram
sequenceDiagram
participant Scheduler
participant Eval as processAlert
participant Queue as Notification Dispatcher
participant Hook as Webhook
participant Mongo as Alert State/History
Scheduler->>Eval: Evaluate alert
Eval->>Queue: dispatch(NotificationJob)
Queue-->>Eval: Enqueued
Eval->>Mongo: Persist state/history
Queue->>Hook: Deliver asynchronously
alt delivery fails
Hook-->>Queue: Error
Queue->>Queue: Buffer failure by alertId
Scheduler->>Eval: Next evaluation
Eval->>Queue: drainDeliveryFailures(alertId)
Queue-->>Eval: WEBHOOK_ERROR
Eval->>Mongo: Persist executionErrors/ERROR history
end
opt external one-shot mode
Scheduler->>Queue: shutdown(60s)
Scheduler->>Scheduler: Exit process
end
Reviews (1): Last reviewed commit: "[HDX-5037] perf(alerts): deliver notific..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await shutdownNotificationDispatcher(60_000); | ||
| process.exit(0); |
There was a problem hiding this comment.
When RUN_SCHEDULED_TASKS_EXTERNALLY is enabled and a queued webhook delivery fails, the failure remains only in the process-local dispatcher while shutdown merely waits for deliveries before exiting. The next evaluation starts with an empty buffer, so the failure never reaches the alert's executionErrors or ERROR history.
Knowledge Base Used: Alerts and Background Tasks
| if (singleton) { | ||
| await singleton.shutdown(deadlineMs); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This newly added 346-line module exceeds the repository's 300-line limit, coupling queue lifecycle, tracing, failure buffering, and singleton management in one file and increasing its review and maintenance cost.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Deep Review✅ No critical issues found. The queue is carefully guarded against unhandled rejections (per- 🟡 P2 — recommended
🔵 P3 nitpicks (3)
Reviewers (10): correctness, reliability, adversarial, security, performance, testing, maintainability, kieran-typescript, project-standards, learnings-researcher. Testing gaps: No test asserts the feedback loop is non-functional (or is intentionally bypassed) under |
E2E Test Results✅ All tests passed • 293 passed • 1 skipped • 1107s
Tests ran across 4 shards in parallel. |
1 participant
Why
Notification deliveries are awaited inline inside
processAlert, so each webhook send occupies an alert-evaluation slot for its full duration — including up to 3 retry attempts with backoff against slow or dead endpoints. On large tenants this extends the check-alerts tick and pushes it into skipped cron fires.Linear: HDX-5037
What
Cross-tick background delivery queue
Evaluations now dispatch fully rendered
NotificationJobs to anInProcessNotificationDispatcherand continue immediately:NotificationJob(notifications.ts): versioned, fully serializable core (zod) designed as a future notification service's wire contract —eventId(the existing alert+channel+groupobjectHash) is the idempotency key, channels are referenced by id. The resolved webhook doc rides along as a transport-local sidecar for in-process delivery.NotificationDispatcherinterface (dispatch/shutdown/drainDeliveryFailures), mirroring theAlertProviderpattern.InlineNotificationDispatcheris the default, so directprocessAlert/renderAlertTemplatecalls without a dispatcher keep today's inline delivery and error semantics.InProcessNotificationDispatcher(notificationQueue.ts): bounded worker pool (concurrency 20); per-eventIdFIFO chaining guarantees a RESOLVED notification never overtakes its still-pending ALERT for the same alert+channel+group; bounded depth (5k) with overflow drops counted onhyperdx.alerts.notifications_dropped; each delivery runs in its own root span linked back to the originating evaluation, with health on thealerts.notification_deliveryoperation SLI.CheckAlertTaskinstances are per cron tick, but deliveries outlive the tick that produced them. The one-shot runner path (RUN_SCHEDULED_TASKS_EXTERNALLY) drains with a 60s deadline before exiting.Delivery-failure feedback into
executionErrorsBecause alert state now persists before the webhook lands, a naive queue would silently drop delivery errors from the alert's
executionErrors— the alert details page would show a permanently failing webhook as perfectly healthy. (We observed exactly this on a deployment running a queued-delivery variant: an alert firing every minute against a dead webhook endpoint, with per-minuteFailed to deliver alert notificationlogs, yetexecutionErrors: []forever.)This PR builds the feedback loop in from the start:
WEBHOOK_ERROR, bounded: 5 most-recent per alert, 10k tracked alerts).drainDeliveryFailures()and merges them into itsexecutionErrors, so the failure surfaces on the alert one tick late and clears once deliveries succeed again. The existing provider logic also upserts them as an ERROR row in the evaluation history — no UI changes needed.recordAlertErrorscalls include drained errors so a failing query tick doesn't wipe them.Accepted semantics vs inline delivery
evaluationAnalytics.webhookDurationMsnow measures dispatch (enqueue) time, not delivery time; delivery latency lives on thealerts.notification_deliverySLI and thedeliverNotificationspan.Also moves
makeAlertError/makeWebhookAlertError/getErrorMessagefromcheckAlerts/index.tsintocheckAlerts/errors.tsso the queue can buildIAlertErrors without an import cycle.Testing
notifications.test.ts/notificationQueue.test.ts(new, unit): job wire-contract round trip; dispatch-before-delivery; per-eventId FIFO; overflow drops; shutdown drain/deadline; no unhandled rejections from broken jobs; failure buffering, per-alert caps, redirect-specific messages, preview-path (no alertId) exclusion.checkAlerts.int.test.ts(integration):WEBHOOK_ERRORinexecutionErrorsand an ERROR history row on the next evaluation, then clears after the endpoint recovers.renderAlertTemplate.int.test.ts(integration): notifications route through a provided dispatcher (nothing sent inline; serializable core validates against the zod contract); inline delivery is preserved when no dispatcher is passed.checkAlerts.int(166 passed),renderAlertTemplate(76 passed),tsc --noEmitclean, eslint at the 302-warning budget.