fix(notifications): fan out alarm destinations#520
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
The latest updates on your projects. Learn more about Unkey Deploy
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 3/5
- In
packages/notifications/src/alarm-config.ts,buildAlarmNotificationConfigappears to overwrite same-channel destinations, which can cause duplicate sends to the last Slack/webhook/email target while silently skipping earlier duplicates; this is user-facing notification behavior, so preserve all duplicate destinations (or explicitly dedupe by target) before merging. - In
apps/uptime/src/uptime-transition-alerts.ts,alarms_firedis still derived from any successful destination instead of per-target successes, so fanout metrics can undercount/overcount real delivery outcomes and mask notification gaps; switch the return/count logic to per-target success accounting before merging.
Architecture diagram
sequenceDiagram
participant Trigger as Notification Trigger (Uptime/Anomaly/Test)
participant TargetBuilder as buildAlarmNotificationTargets
participant Client as NotificationClient (per target)
participant Slack as Slack Webhook
participant Webhook as Custom Webhook
participant Email as Email (Resend)
participant ErrorCapture as Error Logger
Note over Trigger,TargetBuilder: NEW: Fan-out each destination as separate target
Trigger->>TargetBuilder: alarm.destinations[]
TargetBuilder->>TargetBuilder: For each dest:<br/>- sanitize webhook headers<br/>- create {channel, clientConfig}
TargetBuilder-->>Trigger: AlarmNotificationTarget[]
alt No targets (empty array)
Trigger->>Trigger: Return false / skip
else Targets exist
loop Per target
Trigger->>Client: new NotificationClient(target.clientConfig)
Trigger->>Client: send(payload, { channels: [target.channel] })
alt target.channel == "slack"
Client->>Slack: POST webhook (single URL)
Slack-->>Client: response
else target.channel == "webhook"
Client->>Webhook: POST with sanitized headers
Webhook-->>Client: response
else target.channel == "email"
Client->>Email: sendEmailAction (Resend)
Email-->>Client: delivery status
end
Client-->>Trigger: NotificationResult[] (per channel)
alt At least one successful result
Trigger->>Trigger: Mark as success
else All failures
Trigger->>ErrorCapture: captureError({ alarm_id, channel, cause })
Note over Trigger,ErrorCapture: CHANGED: capture per-target with channel context
end
end
Trigger->>Trigger: Aggregate: any success ? true : false
end
Note over Trigger,ErrorCapture: CHANGED: per-target delivery replaces aggregated multi-channel send<br/>Results are now counted by successful target sends
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
Greptile SummaryThis PR fixes the same-channel alarm destination collapse bug where multiple Slack/email/webhook destinations of the same type would collapse to the last config. It introduces
Confidence Score: 4/5The core fan-out logic is correct and all active call sites have been migrated to The fan-out implementation is sound: each destination gets its own
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller as Alarm Trigger
participant BT as buildAlarmNotificationTargets
participant NC1 as NotificationClient (target 1)
participant NC2 as NotificationClient (target 2)
participant P1 as Provider (Slack URL #1)
participant P2 as Provider (Slack URL #2)
Caller->>BT: destinations[]
BT-->>Caller: "[{channel:slack, clientConfig:{url:#1}}, {channel:slack, clientConfig:{url:#2}}]"
par Fan-out (concurrent)
Caller->>NC1: "send(payload, {channels:[slack]})"
NC1->>P1: provider.send(payload)
P1-->>NC1: NotificationResult
NC1-->>Caller: [NotificationResult]
and
Caller->>NC2: "send(payload, {channels:[slack]})"
NC2->>P2: provider.send(payload)
P2-->>NC2: NotificationResult
NC2-->>Caller: [NotificationResult]
end
Caller->>Caller: results.flat() → count successes / log failures
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Caller as Alarm Trigger
participant BT as buildAlarmNotificationTargets
participant NC1 as NotificationClient (target 1)
participant NC2 as NotificationClient (target 2)
participant P1 as Provider (Slack URL #1)
participant P2 as Provider (Slack URL #2)
Caller->>BT: destinations[]
BT-->>Caller: "[{channel:slack, clientConfig:{url:#1}}, {channel:slack, clientConfig:{url:#2}}]"
par Fan-out (concurrent)
Caller->>NC1: "send(payload, {channels:[slack]})"
NC1->>P1: provider.send(payload)
P1-->>NC1: NotificationResult
NC1-->>Caller: [NotificationResult]
and
Caller->>NC2: "send(payload, {channels:[slack]})"
NC2->>P2: provider.send(payload)
P2-->>NC2: NotificationResult
NC2-->>Caller: [NotificationResult]
end
Caller->>Caller: results.flat() → count successes / log failures
Reviews (1): Last reviewed commit: "fix(notifications): fan out alarm destin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function buildAlarmNotificationConfig(destinations: AlarmDestination[]) { | ||
| const clientConfig: Record<string, Record<string, unknown>> = {}; | ||
| const clientConfig: NotificationClientConfig = {}; | ||
| const channels: NotificationChannel[] = []; | ||
|
|
||
| for (const target of buildAlarmNotificationTargets(destinations)) { | ||
| Object.assign(clientConfig, target.clientConfig); | ||
| channels.push(target.channel); | ||
| } | ||
|
|
||
| return { clientConfig, channels }; | ||
| } |
There was a problem hiding this comment.
buildAlarmNotificationConfig now silently fires the last same-channel destination multiple times
The wrapper rebuilds clientConfig with Object.assign, so for two Slack destinations the second URL overwrites the first. channels still gets ["slack", "slack"]. When a caller creates new NotificationClient(clientConfig).send(payload, { channels }), the single Slack provider (URL #2) is invoked twice — URL #1 never fires. This is strictly worse than the pre-PR behavior (last one wins, called once) and is a regression introduced by this wrapper. All callers in this PR have been updated to use buildAlarmNotificationTargets directly, but the exported buildAlarmNotificationConfig is still reachable and silently broken for the case the PR is fixing.
| const results = ( | ||
| await Promise.all( | ||
| targets.map((target) => | ||
| new NotificationClient(target.clientConfig).send(payload, { | ||
| channels: [target.channel], | ||
| }) | ||
| ) | ||
| ) | ||
| ).flat(); | ||
| notificationsSent += results.filter((r) => r.success).length; |
There was a problem hiding this comment.
Promise.all
The uptime code uses Effect.catchTag("NotificationSendError") per target so a single failing destination never aborts the others. Here, if any NotificationClient.send() call throws (e.g., an unexpected error from the email sendEmailAction before the internal Promise.allSettled catches it), the entire Promise.all rejects, leaving notificationsSent under-counted for the current alarm iteration. The alarms.ts test-send path has the same gap. Wrapping each target.send() in .catch(...) (or using Promise.allSettled) would align this with the Effect-based uptime behavior.
|
Addressed the review follow-ups in
Re-ran |
There was a problem hiding this comment.
0 issues found across 6 files (changes from recent commits).
Shadow auto-approve: would require human review. Refactors core notification fan-out logic across uptime alerts, anomaly notifications, and test sends; changes error handling and delivery semantics – high risk for production breakage.
Re-trigger cubic
1 participant
Summary
NotificationResult[]entries and count only successful target sends as firedPR #407 follow-up
apps/uptime/src/uptime-transition-emails.tstoapps/uptime/src/uptime-transition-alerts.tsonstagingVerification
bunx ultracite check packages/notifications/src/alarm-config.ts packages/notifications/src/__tests__/alarm-config.test.ts apps/uptime/src/uptime-transition-alerts.ts packages/rpc/src/routers/alarms.ts packages/rpc/src/routers/anomalies.ts packages/rpc/src/lib/alarm-notifications.ts packages/notifications/src/index.tscd packages/notifications && bun run test && bun run check-typescd apps/uptime && bun run test && bun run check-typescd packages/rpc && bun run check-typesbun run lintbun run check-typesbun run testAI disclosure
Summary by cubic
Fixes alarm destination collisions by sending each destination separately and tracking per-channel results, so multiple Slack/webhook/email targets don’t overwrite each other. Also improves error handling and counts only successful deliveries.
buildAlarmNotificationTargetsin@databuddy/notificationsto fan out destinations; keptbuildAlarmNotificationConfigas a legacy, de-duplicated adapter.sendNotificationTargetin@databuddy/rpc, aggregateNotificationResult[], and sum only successes.Written for commit ce0a06a. Summary will update on new commits.