From 84582ee33d767a14c408f38886672bc42c489e8e Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Sat, 1 Aug 2026 15:55:06 -0700 Subject: [PATCH] fix: alarm channels (#3491) --- config.template.jsonc | 6 +- doc/alarms.md | 46 +++++++-- src/backend/clients/alarm/AlarmClient.test.ts | 99 ++++++++++++++++++- src/backend/clients/alarm/AlarmClient.ts | 73 +++++++++++--- src/backend/clients/alarm/severity.ts | 12 +++ src/backend/clients/alarm/slack.test.ts | 1 + src/backend/clients/alarm/types.ts | 18 +++- src/backend/server.ts | 3 + src/backend/types.ts | 8 +- 9 files changed, 236 insertions(+), 30 deletions(-) diff --git a/config.template.jsonc b/config.template.jsonc index 1902f4489..d80a2d5d7 100644 --- a/config.template.jsonc +++ b/config.template.jsonc @@ -292,8 +292,12 @@ // Optional; defaults to the channel the webhook was created for. "channel": "#alerts", "username": "puter-alarms", - // Lowest severity posted. Default "info" (everything). + // Severity window posted to Slack. The ceiling defaults to + // "info" when PagerDuty is configured — what pages belongs in + // the pager, not in chat — and to "critical" when Slack is the + // only transport. "minSeverity": "info", + "maxSeverity": "info", // Don't repost the same alarm id within this window. The first // occurrence always posts; the next post that gets through // reports how many occurrences piled up. 0 disables throttling. diff --git a/doc/alarms.md b/doc/alarms.md index c061e4a35..76a620722 100644 --- a/doc/alarms.md +++ b/doc/alarms.md @@ -15,15 +15,17 @@ this.clients.alarm.create( ## Severity is the routing decision -| Severity | Meaning | Goes to | -| ---------- | ------------------------------------------------------ | ------------------ | -| `critical` | An unhandled server error. Someone gets woken up. | Pager + chat | -| `error` | Same urgency as critical; prefer one of the other two. | Pager + chat | -| `warning` | Worth a look today. Nobody is paged. | Pager (low) + chat | -| `info` | A record of something expected-but-notable. | Chat only | +| Severity | Meaning | Goes to | +| ---------- | ------------------------------------------------------ | ----------- | +| `critical` | An unhandled server error. Someone gets woken up. | Pager | +| `error` | Same urgency as critical; prefer one of the other two. | Pager | +| `warning` | Worth a look today. Nobody is paged. | Pager (low) | +| `info` | A record of something expected-but-notable. | Chat | -Each transport declares the lowest severity it accepts, so the value a call -site passes is what decides where the alarm lands. The bar for `critical` is +Each transport declares the severity window it accepts, so the value a call +site passes is what decides where the alarm lands. The two windows don't +overlap by default: anything that pages lives in the paging system, and chat +is the record of what didn't. The bar for `critical` is deliberately high: an unhandled 5xx out of the HTTP error handler is the main thing that still pages. Anything a human can look at tomorrow is `warning`, and anything that's just worth recording is `info`. @@ -42,6 +44,29 @@ An extension whose signals are all one tier can default its own local `raiseAlarm` helper to that tier instead of repeating it at every call site — see [extensions/cronMonitor](../../../extensions/cronMonitor/index.js). +## One incident per occurrence, unless you say otherwise + +Alarms always de-dupe *in process* — repeats of an id bump its occurrence +count rather than creating a second alarm. What that means for the pager is a +separate decision, and by default every occurrence opens its own PagerDuty +incident: two failed scans an hour apart are two things that happened, and +closing one shouldn't hide the other. + +Pass `{ dedup: true }` as a fifth argument when repeats of the id really are +one recurring fault, and they collapse onto a single incident carrying the +occurrence count: + +```ts +this.clients.alarm.create(alarmId, message, fields, 'critical', { + dedup: true, +}); +``` + +The HTTP error handler uses it: its id is route + error signature, so a hot +loop of the same crash is one incident with N occurrences instead of N pages. +Reach for it anywhere else only when the id is that specific — otherwise a +per-request alarm can flood the pager. + ## Configuration Everything lives under `pager` in config (see @@ -59,11 +84,16 @@ alarms to the console. "webhookUrl": "…", "channel": "#alerts", "minSeverity": "info", + "maxSeverity": "info", "repeatThrottleMs": 900000, }, } ``` +Slack's `maxSeverity` defaults to `info` whenever PagerDuty is configured, and +to `critical` when it isn't — a node with only a webhook still sees +everything. Raise it to have chat mirror the paging tiers as well. + ### Retiering without a deploy `severityOverrides` is the escape hatch for an alarm that turns out to be diff --git a/src/backend/clients/alarm/AlarmClient.test.ts b/src/backend/clients/alarm/AlarmClient.test.ts index 810bfe3d9..fb23e728b 100644 --- a/src/backend/clients/alarm/AlarmClient.test.ts +++ b/src/backend/clients/alarm/AlarmClient.test.ts @@ -13,13 +13,14 @@ const makeClient = (pager: IPagerConfig = {}) => const capture = ( client: AlarmClient, minSeverity?: 'critical' | 'error' | 'warning' | 'info', + maxSeverity?: 'critical' | 'error' | 'warning' | 'info', ) => { const seen: AlertPayload[] = []; client.addAlertHandler( async (alert) => { seen.push(alert); }, - { name: 'capture', minSeverity }, + { name: 'capture', minSeverity, maxSeverity }, ); return seen; }; @@ -62,6 +63,16 @@ describe('AlarmClient severity routing', () => { expect(chat).toHaveLength(1); }); + it('skips handlers whose ceiling the alarm exceeds', () => { + const client = makeClient(); + const chat = capture(client, 'info', 'info'); + + client.create('rate-limit', 'a user hit a limit', {}, 'info'); + client.create('outage', 'everything is on fire', {}, 'critical'); + + expect(chat.map((alert) => alert.id)).toEqual(['rate-limit']); + }); + it('retiers an alarm from config', () => { const client = makeClient({ severityOverrides: { 'noisy:*': 'info' }, @@ -181,7 +192,7 @@ describe('AlarmClient transport registration', () => { pdEvent.mockClear(); }); - it('keeps info alarms out of PagerDuty but sends them to Slack', async () => { + it('splits info to Slack and everything above it to PagerDuty', async () => { const fetchMock = vi.fn(async () => ({ ok: true, status: 200 })); vi.stubGlobal('fetch', fetchMock); @@ -197,11 +208,93 @@ describe('AlarmClient transport registration', () => { client.create('loud:thing', 'paging', {}, 'critical'); await vi.waitFor(() => expect(pdEvent).toHaveBeenCalledTimes(1)); - expect(fetchMock).toHaveBeenCalledTimes(2); + // The pager has it; chat doesn't repeat it. + expect(fetchMock).toHaveBeenCalledTimes(1); vi.unstubAllGlobals(); }); + it('sends every severity to Slack when there is no pager', async () => { + const fetchMock = vi.fn(async () => ({ ok: true, status: 200 })); + vi.stubGlobal('fetch', fetchMock); + + const client = makeClient({ + slack: { enabled: true, webhookUrl: 'https://hooks.example/abc' }, + }); + await client.onServerStart(); + + client.create( + 'loud:thing', + 'nowhere else to send this', + {}, + 'critical', + ); + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); + + vi.unstubAllGlobals(); + }); + + it('lets config widen the Slack ceiling back out', async () => { + const fetchMock = vi.fn(async () => ({ ok: true, status: 200 })); + vi.stubGlobal('fetch', fetchMock); + + const client = makeClient({ + pagerduty: { enabled: true, routingKey: 'rk' }, + slack: { + enabled: true, + webhookUrl: 'https://hooks.example/abc', + maxSeverity: 'critical', + }, + }); + await client.onServerStart(); + + client.create('loud:thing', 'paging', {}, 'critical'); + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); + + vi.unstubAllGlobals(); + }); + + it('gives each occurrence its own PagerDuty incident by default', async () => { + const client = makeClient({ + pagerduty: { enabled: true, routingKey: 'rk' }, + }); + await client.onServerStart(); + + client.create('scan:failed', 'first', {}, 'warning'); + client.create('scan:failed', 'second', {}, 'warning'); + await vi.waitFor(() => expect(pdEvent).toHaveBeenCalledTimes(2)); + + const keys = pdEvent.mock.calls.map( + ([arg]: [{ data: { dedup_key: string } }]) => arg.data.dedup_key, + ); + expect(keys[0]).not.toBe(keys[1]); + }); + + it('collapses repeats of a dedup alarm onto one incident', async () => { + const client = makeClient({ + pagerduty: { enabled: true, routingKey: 'rk' }, + }); + await client.onServerStart(); + + const raise = () => + client.create( + 'http_500:POST:/notif/mark-ack:deadlock', + 'HTTP 500 on POST /notif/mark-ack: deadlock', + {}, + 'critical', + { dedup: true }, + ); + raise(); + raise(); + await vi.waitFor(() => expect(pdEvent).toHaveBeenCalledTimes(2)); + + const keys = pdEvent.mock.calls.map( + ([arg]: [{ data: { dedup_key: string } }]) => arg.data.dedup_key, + ); + expect(keys[0]).toBe('http_500:POST:/notif/mark-ack:deadlock'); + expect(keys[1]).toBe(keys[0]); + }); + it('skips transports that are enabled but not configured', async () => { const client = makeClient({ pagerduty: { enabled: true }, diff --git a/src/backend/clients/alarm/AlarmClient.ts b/src/backend/clients/alarm/AlarmClient.ts index 33315f433..ea245f966 100644 --- a/src/backend/clients/alarm/AlarmClient.ts +++ b/src/backend/clients/alarm/AlarmClient.ts @@ -22,11 +22,16 @@ import { inspect } from 'node:util'; import { createHash } from 'node:crypto'; import type { IConfig, PagerSeverity, SeverityRule } from '../../types'; import { PuterClient } from '../types'; -import { meetsMinSeverity, resolveSeverityOverride } from './severity'; +import { + meetsMinSeverity, + resolveSeverityOverride, + withinMaxSeverity, +} from './severity'; import { createSlackAlertHandler } from './slack'; import type { Alarm, AlarmFields, + AlarmOptions, AlertHandler, AlertPayload, KnownErrorRule, @@ -35,6 +40,7 @@ import type { export type { Alarm, AlarmFields, + AlarmOptions, AlertHandler, AlertPayload, KnownErrorRule, @@ -47,6 +53,8 @@ interface RegisteredHandler { name: string; /** Lowest severity this transport accepts. */ minSeverity: PagerSeverity; + /** Highest severity this transport accepts. */ + maxSeverity: PagerSeverity; handler: AlertHandler; } @@ -54,6 +62,8 @@ interface RegisteredHandler { const FALLBACK_SEVERITY: PagerSeverity = 'critical'; /** Keeps `info` alarms out of the paging system unless config says otherwise. */ const DEFAULT_PAGERDUTY_MIN_SEVERITY: PagerSeverity = 'warning'; +/** Slack's ceiling once a pager exists: chat gets what doesn't page. */ +const DEFAULT_SLACK_MAX_SEVERITY_WITH_PAGER: PagerSeverity = 'info'; // -- Helpers ---------------------------------------------------------- @@ -153,7 +163,7 @@ function cleanFields(fields: AlarmFields): Record { /** * Manages system alarms and routes them to alert transports by severity. * - * Severity is the routing decision: each transport declares the lowest severity + * Severity is the routing decision: each transport declares the severity window * it accepts, so `critical` pages on-call while `info` only reaches the chat * channel. Config can retier or mute any alarm id after the fact — see * `pager.severityOverrides` in {@link IConfig}. @@ -173,8 +183,8 @@ export class AlarmClient extends PuterClient { // -- Lifecycle ---------------------------------------------------- override async onServerStart(): Promise { - this.registerPagerDuty(); - this.registerSlack(); + const paging = this.registerPagerDuty(); + this.registerSlack({ paging }); } override onServerPrepareShutdown(): void { @@ -183,16 +193,17 @@ export class AlarmClient extends PuterClient { console.log('[alarm] entering drain mode — suppressing new alarms'); } - private registerPagerDuty(): void { + /** @returns Whether a working PagerDuty transport was registered. */ + private registerPagerDuty(): boolean { const pagerDutyConf = this.config.pager?.pagerduty; - if (!pagerDutyConf?.enabled) return; + if (!pagerDutyConf?.enabled) return false; const routingKey = pagerDutyConf.routingKey; if (!routingKey) { console.warn( '[alarm] PagerDuty enabled but no routingKey configured', ); - return; + return false; } const serverId = this.config.serverId; @@ -205,7 +216,7 @@ export class AlarmClient extends PuterClient { data: { routing_key: routingKey, event_action: 'trigger', - dedup_key: alert.id, + dedup_key: alert.dedupKey, payload: { summary: alert.message, source: alert.source, @@ -224,9 +235,10 @@ export class AlarmClient extends PuterClient { console.log( `[alarm] PagerDuty handler registered (min severity: ${minSeverity})`, ); + return true; } - private registerSlack(): void { + private registerSlack({ paging }: { paging: boolean }): void { const slackConf = this.config.pager?.slack; if (!slackConf?.enabled) return; @@ -236,16 +248,23 @@ export class AlarmClient extends PuterClient { } const minSeverity = slackConf.minSeverity ?? 'info'; + // With a pager taking everything from `warning` up, chat is where the + // rest is recorded — reposting the paging tiers there only trains + // people to skim the channel. Without one, Slack is the only place an + // alarm can land, so it takes all of them. + const maxSeverity = + slackConf.maxSeverity ?? + (paging ? DEFAULT_SLACK_MAX_SEVERITY_WITH_PAGER : 'critical'); this.addAlertHandler( createSlackAlertHandler(slackConf, { serverId: this.config.serverId, }), - { name: 'slack', minSeverity }, + { name: 'slack', minSeverity, maxSeverity }, ); console.log( - `[alarm] Slack handler registered (min severity: ${minSeverity})`, + `[alarm] Slack handler registered (severity ${minSeverity}..${maxSeverity})`, ); } @@ -265,12 +284,16 @@ export class AlarmClient extends PuterClient { * Omit it to take `pager.defaultSeverity` (itself defaulting to * 'critical'). Operators can retier or mute any alarm id from config * afterwards, so the value here is the starting point, not the last word. + * + * Pass `{ dedup: true }` when repeats of this id are one recurring fault + * that should collapse into a single incident — see {@link AlarmOptions}. */ create( id: string, message: string, fields: AlarmFields = {}, severity?: PagerSeverity, + opts: AlarmOptions = {}, ): void { if (this.draining) { if (!this.drainLogged) { @@ -294,6 +317,7 @@ export class AlarmClient extends PuterClient { message, fields, severity, + dedup: opts.dedup, started: Date.now(), // `recordOccurrence` below stamps the first occurrence; seeding one // here too would report every alarm as one occurrence ahead. @@ -325,16 +349,22 @@ export class AlarmClient extends PuterClient { /** * Register an additional alert handler. Handlers are called for every alarm - * that isn't suppressed by a known-error rule or muted by config, and that - * meets the handler's own `minSeverity` (default: everything). + * that isn't suppressed by a known-error rule or muted by config, and whose + * severity falls inside the handler's own `minSeverity`..`maxSeverity` + * window (default: everything). */ addAlertHandler( handler: AlertHandler, - opts: { name?: string; minSeverity?: PagerSeverity } = {}, + opts: { + name?: string; + minSeverity?: PagerSeverity; + maxSeverity?: PagerSeverity; + } = {}, ): void { this.alertHandlers.push({ name: opts.name ?? `handler-${this.alertHandlers.length}`, minSeverity: opts.minSeverity ?? 'info', + maxSeverity: opts.maxSeverity ?? 'critical', handler, }); } @@ -448,8 +478,17 @@ export class AlarmClient extends PuterClient { const fieldsClean = cleanFields(alarm.fields); const repeatCount = alarm.timestamps.length; + const id = alarm.id || 'something-bad'; + const payload: AlertPayload = { - id: alarm.id || 'something-bad', + id, + // A de-duplicating transport should fold repeats of a recurring + // fault into one incident, but everything else is a fresh event + // each time it fires — the start time keeps occurrence numbering + // from colliding with an incident left open by an earlier boot. + dedupKey: alarm.dedup + ? id + : `${id}#${alarm.started}.${repeatCount}`, shortId: alarm.shortId, message: alarm.message || alarm.id || 'something bad happened', source: 'alarm', @@ -465,8 +504,10 @@ export class AlarmClient extends PuterClient { }, }; - for (const { name, minSeverity, handler } of this.alertHandlers) { + for (const { name, minSeverity, maxSeverity, handler } of this + .alertHandlers) { if (!meetsMinSeverity(resolved, minSeverity)) continue; + if (!withinMaxSeverity(resolved, maxSeverity)) continue; handler(payload).catch((err) => { console.error( `[alarm] ${name} alert handler failed: ${err?.message}`, diff --git a/src/backend/clients/alarm/severity.ts b/src/backend/clients/alarm/severity.ts index 560f4062c..45306818c 100644 --- a/src/backend/clients/alarm/severity.ts +++ b/src/backend/clients/alarm/severity.ts @@ -41,6 +41,18 @@ export function meetsMinSeverity( return RANK[severity] >= RANK[min]; } +/** + * True when `severity` is quiet enough for a transport whose ceiling is `max`. + * A ceiling is what keeps a chat transport from repeating everything the pager + * already delivered. + */ +export function withinMaxSeverity( + severity: PagerSeverity, + max: PagerSeverity, +): boolean { + return RANK[severity] <= RANK[max]; +} + /** * Look up the operator override for an alarm id. Exact ids win over prefix * patterns (`cronMonitor:*`); among patterns the longest prefix wins, so a diff --git a/src/backend/clients/alarm/slack.test.ts b/src/backend/clients/alarm/slack.test.ts index 32cad2b16..0ab0873aa 100644 --- a/src/backend/clients/alarm/slack.test.ts +++ b/src/backend/clients/alarm/slack.test.ts @@ -4,6 +4,7 @@ import type { AlertPayload } from './types'; const alert = (over: Partial = {}): AlertPayload => ({ id: 'cronMonitor:high_aiLogEntries', + dedupKey: 'cronMonitor:high_aiLogEntries#1.1', shortId: 'amber-delta-fox', message: 'High AI log entries: 1200 in the last 10 minutes', source: 'alarm', diff --git a/src/backend/clients/alarm/types.ts b/src/backend/clients/alarm/types.ts index d90a04a0d..66a9c113e 100644 --- a/src/backend/clients/alarm/types.ts +++ b/src/backend/clients/alarm/types.ts @@ -30,7 +30,18 @@ export interface AlarmOccurrence { timestamp: number; } -export interface Alarm { +export interface AlarmOptions { + /** + * Collapse repeats of this alarm id into a single incident on transports + * that de-duplicate (PagerDuty). Only right when the id already pins down + * one recurring fault — an uncaught error on a route, say — so that N + * occurrences really are one thing to fix. Off by default: every other + * alarm raises its own incident per occurrence. + */ + dedup?: boolean; +} + +export interface Alarm extends AlarmOptions { id: string; shortId: string; message: string; @@ -47,6 +58,11 @@ export interface Alarm { export interface AlertPayload { id: string; + /** + * Key a de-duplicating transport groups by. Equal to `id` for alarms raised + * with `dedup`, and unique per occurrence for everything else. + */ + dedupKey: string; /** Readable slug for the same alarm, for humans quoting it back. */ shortId: string; message: string; diff --git a/src/backend/server.ts b/src/backend/server.ts index bd52682cf..4218043dd 100644 --- a/src/backend/server.ts +++ b/src/backend/server.ts @@ -781,6 +781,9 @@ export class PuterServer { // An unhandled server error is the one thing that // still pages on-call. forcedSeverity ?? 'critical', + // The id pins route + error signature, so repeats are + // the same fault and belong on one incident. + { dedup: true }, ); }, }), diff --git a/src/backend/types.ts b/src/backend/types.ts index f400488a0..825a61a1f 100644 --- a/src/backend/types.ts +++ b/src/backend/types.ts @@ -97,8 +97,14 @@ export interface ISlackAlertConfig { channel?: string; /** Bot display name on the posted message. */ username?: string; - /** Lowest severity posted to Slack. Default `info` (everything). */ + /** Lowest severity posted to Slack. Default `info`. */ minSeverity?: PagerSeverity; + /** + * Highest severity posted to Slack. Defaults to `info` when PagerDuty is + * configured — anything that pages belongs in the paging system, not in + * chat — and to `critical` (everything) when Slack is the only transport. + */ + maxSeverity?: PagerSeverity; /** * Don't repost the same alarm id within this window. The first occurrence * always posts; repeats inside the window only bump the occurrence count