diff --git a/HISTORY.md b/HISTORY.md index ca1a582..0bf9e4a 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -7451,3 +7451,109 @@ FAIL modal > тло стає inert, поки вікно відкрите драбини — ніколи; втрата advisory-lock може дати подвійне сповіщення, бо `ApplyEscalation` не звіряє оренду; та сама відсутність перевірки каналів живе в `alr.rules.channel_ids` і `alr.routes.channel_ids`. + +--- + +## 2026-08-28 — Рецензія виправлень: половина була закрита лише наполовину + +Рецензія попереднього коміту знайшла, що виправлення пересунули вади, а +не прибрали. Усе нижче — наслідок саме цієї другої рецензії. + +### Мій власний скрипт не мав запобіжника, про який сам писав + +`scripts/dbtest.sh` у шапці стверджував «напрямляти на робочу базу не +можна» — і не робив нічого, щоб це забезпечити. Перевірено фактично: +`NETPULSE_TEST_DSN=.../netpulse` пішов котити міграції на базу з бойовим +іменем, і зупинила його лише відсутність сервера на тому порту. Забута в +профілі шелла змінна — і міграції з гілки лягають на робочу базу. + +Тепер ім'я бази мусить містити `probe` або `test`, інакше відмова з +поясненням. Перевірка навмисно дурна й невідмикна: розумніша +(маркерна таблиця, прапорець згоди) перекладає рішення на людину рівно +в той момент, коли людина поспішає. Плюс ім'я контейнера з номером +процесу (два прогони поруч не вбивають бази один одному) і збереження +бази після ЧЕРВОНОГО прогону — подивитись, що в таблицях, інакше +неможливо саме тоді, коли треба. + +### Журнал усе ще брехав — тепер про доставку + +Попереднє виправлення звузило брехню з «канали не прочитались» до +«канали прочитались, доставка впала»: `sent` рахував проходження +фільтра, бо `sendText` ковтав помилку й нічого не повертав. Telegram +лежить десять хвилин — у журналі «надіслано», а в `alr.notifications` +поруч `failed`. Два журнали суперечили один одному. + +`sendText` повертає помилку; сходинка розрізняє три результати: +`no_channels` (не було кому слати), `failed` (слали, не дійшло), `sent`. +Заодно остання сходинка проходу пише в журнал `sent`, а не `done`: +`done` правильне як причина зупинки драбини, але підрахунок «скільки +разів реально слали» через нього щоразу недорахував по одному. + +### `stopped_at IS NULL` закривав лише один шлях із чотирьох + +Умова рятувала від «Прийняти» й ручного закриття — бо тільки ці шляхи +ставлять `stopped_at`. А `resolveRuleAlerts` (вимкнення чи видалення +правила) і `ResolveMissing` (метрика відновилась) рядка драбини не +чіпають: сходинка, взята в партію до гасіння, дзвонила за погашеним +алертом. + +Тепер сходинка, що має спрацювати, вимагає ще й `алерт усе ще firing` — +одним `EXISTS` у тому ж `UPDATE`. Умова стоїть тут, а не в кожному з +шляхів гасіння, саме тому, що їх багато й побільшає: перевіряти стан у +момент дії надійніше, ніж пам'ятати про драбину в кожному новому місці. +Драбина при цьому не застрягає — наступний такт бачить `resolved` і +зупиняє її штатно (є тест). + +### Ескалація більше не блокує весь движок + +`escalate()` жив у тіку движка ПЕРЕД обчисленням правил. Доставка +синхронна, до сотні сходинок у партії, десятки секунд таймауту на +мертвому вебхуці — тобто один кабінет із непрацюючим каналом зупиняв +обчислення правил УСІМ: нові аварії не піднімались, перші сповіщення не +йшли. Механізм, який існує, щоб аварію точно помітили, робив аварії +непомітними. + +Тепер `RunEscalations` — свій такт. Своє блокування не потрібне: черга +розбирається через `FOR UPDATE SKIP LOCKED` плюс оренда рядка. + +### Аварія всередині вікна обслуговування не будила нікого й ніколи + +Алерт, народжений заглушеним, не отримував сповіщення (бо заглушений), а +коли вікно закінчувалось — теж не отримував, бо вже «не новий»: перехід +`suppressed → firing` лише перемальовував екран. Драбина не взводилась +теж. Аварія о 03:00 усередині вікна 02:30–03:30 лишалась невидимою до +кінця свого життя. + +Тепер такий перехід сповіщається нарівні з новим алертом. І відлік +драбини ведеться від ПЕРШОГО СПОВІЩЕННЯ, а не від `started_at`: інакше +драбина протухла б ще у вікні й висипалась одним залпом. «П'ятнадцять +хвилин на підтвердження» починаються тоді, коли людина вперше могла щось +зробити. + +### Канали правил: та сама вада, той самий спосіб + +`alr.rules.channel_ids` (`uuid[]`, FK неможливий) приймав будь-які UUID, +зокрема з чужого кабінету. Наслідок гірший, ніж у драбині: `targets()` +не знаходить канал, `sent == 0` — і через це не взводиться навіть +драбина. Одна помилка в id глушила і сповіщення, і резервний механізм. +Перевірка закрита у store тим самим способом, що для сходинок. +`DeleteChannel` тепер чистить посилання і в правилах, і в маршрутах. + +`alr.routes` виявились без жодного шляху запису — екрана й обробника +немає, покласти туди чужий id можна лише прямим SQL. Перевірку туди не +додавали: не було б куди її поставити. + +### І те, заради чого все це — прогін проти справжньої бази + +`dbtest.sh` одразу впіймав те, чого не бачив жоден тест на структурах: +`channel_ids uuid[] NOT NULL DEFAULT '{}'` виглядає безпечним, але +DEFAULT спрацьовує лише на ВІДСУТНІЙ стовпець — nil-зріз із Go їде явним +NULL і валить запис. Правило без власних каналів (типовий стан: +«сповіщати за загальними маршрутами») давало 500 на цілком звичайній дії. + +**Що лишається відкритим:** ескалація ігнорує тиху годину правила — алерт +о 21:59 ескалює всю ніч, о 22:01 не ескалює ніколи (потрібне рішення, що +тиха година означає для драбини); втрата advisory-lock теоретично дає +подвійне сповіщення (`ApplyEscalation` не звіряє оренду токеном); +`LoadChannels` падає цілком через один нерозшифровний секрет, і кабінет +лишається без ескалацій до стелі життя драбини. diff --git a/scripts/dbtest.sh b/scripts/dbtest.sh index 7ce6423..ef0c68c 100644 --- a/scripts/dbtest.sh +++ b/scripts/dbtest.sh @@ -15,25 +15,48 @@ # тож викликається окремо: перед розгортанням і в CI. # # БАЗА МУСИТЬ БУТИ ОДНОРАЗОВОЮ. Тести пишуть, видаляють і перемикають -# ролі; напрямляти їх на робочу базу не можна. Скрипт свою базу створює -# сам і дропає на початку кожного прогону. +# ролі, а перед ними ще й котяться міграції — зокрема ті, яких у робочій +# базі ще не має бути. І це не побажання в коментарі: рівно так гине +# бойова база, коли в профілі шелла лишився експортований +# NETPULSE_TEST_DSN зі вчорашнього налагодження. Тому нижче стоїть +# перевірка імені, а не заклик до обережності. # # ВИКОРИСТАННЯ -# scripts/dbtest.sh # підніме свій Postgres у Docker -# NETPULSE_TEST_DSN=... scripts/dbtest.sh # проти готової бази +# scripts/dbtest.sh # підніме свій Postgres +# NETPULSE_TEST_DSN=... scripts/dbtest.sh # проти готової бази, +# # ім'я якої містить probe/test # set -u SRC="$(cd "$(dirname "$0")/.." && pwd)" OWN_DB=0 +NAME="" + +# Ім'я бази має саме казати, що вона одноразова. Перевірка навмисно +# дурна й невідмикна: розумніша (маркерна таблиця, прапорець згоди) +# перекладає рішення на людину рівно в той момент, коли людина поспішає. +guard_disposable() { + db=$(printf '%s' "$1" | sed -e 's/?.*$//' -e 's#.*/##') + case "$db" in + *probe*|*test*) return 0 ;; + esac + echo "!!! NETPULSE_TEST_DSN вказує на базу «$db»." >&2 + echo " Тести пишуть, видаляють і котять міграції — база мусить бути" >&2 + echo " одноразовою, і її імʼя має містити «probe» або «test»." >&2 + echo " Якщо це чернетка — перейменуйте базу. Якщо ні — ви щойно мало" >&2 + echo " не накотили міграції на робочу базу." >&2 + exit 1 +} if [ -z "${NETPULSE_TEST_DSN:-}" ]; then # Своя база на час прогону. Порт випадковий-таки ні: фіксований, але # нетиповий, щоб не зіткнутись із локальним Postgres розробника. PORT=${NETPULSE_TEST_PORT:-55433} - NAME=netpulse-dbtest + # Імʼя з номером процесу: два прогони поруч (другий термінал, дві + # задачі на одному раннері) інакше вбивали б бази один одному, і + # падіння виглядало б як помилка зʼєднання нізвідки. + NAME="netpulse-dbtest-$$" echo "== піднімаю одноразовий Postgres :$PORT" - docker rm -f "$NAME" >/dev/null 2>&1 docker run -d --name "$NAME" -p "$PORT:5432" \ -e POSTGRES_USER=netpulse -e POSTGRES_PASSWORD=probe \ -e POSTGRES_DB=netpulse_probe \ @@ -46,10 +69,23 @@ if [ -z "${NETPULSE_TEST_DSN:-}" ]; then n=$((n+1)); [ "$n" -gt 60 ] && { echo "база не піднялась"; exit 1; } sleep 1 done +else + guard_disposable "$NETPULSE_TEST_DSN" fi +# Своя база зноситься лише після ЗЕЛЕНОГО прогону. Після червоного вона +# лишається: подивитись, що саме опинилось у таблицях, — єдиний спосіб +# зрозуміти половину падінь, а знесена база забирає цю можливість рівно +# тоді, коли вона потрібна. cleanup() { - [ "$OWN_DB" = 1 ] && docker rm -f netpulse-dbtest >/dev/null 2>&1 + [ "$OWN_DB" = 1 ] || return 0 + if [ "${rc:-1}" = 0 ]; then + docker rm -f "$NAME" >/dev/null 2>&1 + else + echo + echo "база лишилась для розбору: docker exec -it $NAME psql -U netpulse netpulse_probe" + echo "прибрати: docker rm -f $NAME" + fi return 0 } trap cleanup EXIT INT TERM diff --git a/server/cmd/netpulse-api/main.go b/server/cmd/netpulse-api/main.go index 52ccdca..502d701 100644 --- a/server/cmd/netpulse-api/main.go +++ b/server/cmd/netpulse-api/main.go @@ -199,6 +199,7 @@ func run() error { // ніколи не спрацює. api = api.WithEventAlerts(alerting.NewEventSink(st, log)) go eng.Run(ctx) + go eng.RunEscalations(ctx) go eng.RunHousekeeping(ctx, *alertKeep) // Приймач натискань кнопок під сповіщеннями Telegram. diff --git a/server/internal/alerting/engine.go b/server/internal/alerting/engine.go index b4b415c..95ed0fb 100644 --- a/server/internal/alerting/engine.go +++ b/server/internal/alerting/engine.go @@ -110,12 +110,6 @@ func (e *Engine) tick(ctx context.Context) (int, error) { e.expireEvents(ctx) e.deliverPending(ctx) - // Ескалація теж не залежить від наявності метричних правил: драбину - // може взвести подієвий алерт у кабінеті, де опитуваних правил немає - // зовсім. І так само, як дві дії вище, вона мусить статись до - // перевірки на порожній перелік правил. - e.escalate(ctx) - rules, err := e.st.ActiveRules(ctx) if err != nil { return 0, fmt.Errorf("читання правил: %w", err) @@ -168,7 +162,7 @@ func (e *Engine) tickTenant(ctx context.Context, tenantID string, rules []store. } } - var fired, resolved, changed []store.Alert + var fired, resolved, changed, unsuppressed []store.Alert for _, r := range rules { cands, err := e.st.EvaluateRule(ctx, r) @@ -203,6 +197,18 @@ func (e *Engine) tickTenant(ctx context.Context, tenantID string, rules []store. // на екрані алерт має піти в «придушені» без чекання на // наступне перезавантаження сторінки. changed = append(changed, f.Alert) + if leftSuppression(f.PrevState, f.State) { + // Вікно обслуговування скінчилось, а проблема — ні. + // + // Досі такий алерт лише перемальовувався на екрані: + // сповіщення при народженні не пішло (бо заглушено), + // а тут не йшло, бо «не новий». Тобто аварія, яка + // почалась о 03:00 усередині вікна 02:30–03:30, не + // будила нікого й ніколи — ні першим сповіщенням, ні + // драбиною. Це та сама тиха відмова, тільки з + // поважним на вигляд приводом. + unsuppressed = append(unsuppressed, f.Alert) + } } } @@ -225,6 +231,10 @@ func (e *Engine) tickTenant(ctx context.Context, tenantID string, rules []store. notify = append(notify, a) } } + // Ті, з кого щойно зняли заглушення, сповіщаються нарівні з новими: + // для людини це перша звістка про проблему, хоч би скільки вона вже + // тривала за зачиненими дверима. + notify = append(notify, unsuppressed...) if len(notify) > 0 { e.notifier.Dispatch(ctx, tenantID, notify, e.ring) } @@ -372,6 +382,44 @@ func (e *Engine) deliverPending(ctx context.Context) { // половина відповіді ще є, а друга вже стерта. const escalationLogKeep = 90 * 24 * time.Hour +// leftSuppression — чи алерт щойно вийшов із заглушення в бойовий стан. +// +// Винесено окремою функцією не заради краси: це рішення про те, кого +// розбудити, а перевірити його всередині тіку можна лише піднявши базу, +// вікно обслуговування й годинник. Пари станів тут коштують чийогось +// сну в обидва боки — і «не сповістили, бо вважали продовженням», і +// «сповістили вдруге про те саме». +func leftSuppression(prev, cur string) bool { + return prev == "suppressed" && cur == "firing" +} + +// RunEscalations — окремий такт для драбин ескалації. +// +// Окремий, а не всередині tick(), і це не косметика. Доставка сходинки +// синхронна, кожен канал має свій таймаут (десятки секунд на мертвому +// вебхуці), а сходинок у партії до сотні. Поки escalate() жив у тіку +// движка, один кабінет із непрацюючим каналом зупиняв обчислення правил +// УСІМ: нові аварії не піднімались, перші сповіщення не йшли. Тобто +// механізм, який існує, щоб аварію точно помітили, робив аварії +// непомітними. +// +// Своє блокування тут не потрібне: черга розбирається через +// FOR UPDATE SKIP LOCKED плюс оренда рядка, тож два інстанси не візьмуть +// ту саму сходинку — на відміну від обчислення правил, яке саме тому й +// сидить під advisory-блокуванням. +func (e *Engine) RunEscalations(ctx context.Context) { + t := time.NewTicker(e.interval) + defer t.Stop() + for { + select { + case <-ctx.Done(): + return + case <-t.C: + e.escalate(ctx) + } + } +} + // RunHousekeeping переносить закриті алерти в історію. func (e *Engine) RunHousekeeping(ctx context.Context, keepResolved time.Duration) { t := time.NewTicker(15 * time.Minute) diff --git a/server/internal/alerting/escalation.go b/server/internal/alerting/escalation.go index 7004545..6e2d8e3 100644 --- a/server/internal/alerting/escalation.go +++ b/server/internal/alerting/escalation.go @@ -36,9 +36,14 @@ const escalationBatch = 100 // escalate проганяє сходинки, час яких настав. // -// Викликається з тіку движка, тобто під тим самим advisory-блокуванням, -// що й решта. Оренда на рядку (EscalationLease) — другий рубіж на -// випадок, коли блокування з якоїсь причини взяли двоє. +// Викликається зі СВОГО такту (RunEscalations), а не з тіку движка: +// доставка синхронна й повільна, і поки вона жила в тіку, мертвий канал +// одного кабінету затримував обчислення правил усім. +// +// Advisory-блокування тут немає й не треба: TakeDueEscalations розбирає +// чергу через FOR UPDATE SKIP LOCKED, а оренда на рядку +// (EscalationLease) — другий рубіж на випадок, коли рядок усе-таки взяли +// двоє. func (e *Engine) escalate(ctx context.Context) { due, err := e.st.TakeDueEscalations(ctx, escalationBatch) if err != nil { @@ -98,16 +103,29 @@ func (e *Engine) escalate(ctx context.Context) { continue } - if sent := e.notifier.deliverEscalation(ctx, snap, d, byID); sent == 0 { - // Сходинка списана — інакше вона поверталася б щотіку. Але в - // журнал іде правда, а не намір: «надіслано» на сходинці, яка - // нікуди не пішла, — саме та мовчазна відмова, від якої - // ескалація рятує. + // Сходинка вже списана — інакше вона поверталася б щотіку. Але в + // журнал іде правда, а не намір: «надіслано» на сходинці, яка + // нікуди не пішла, — саме та мовчазна відмова, від якої ескалація + // рятує. Тому три різні результати, а не два: + // no_channels — не було кому слати (канал видалили, вимкнули, + // підняли поріг серйозності); + // failed — слали, і жодне не дійшло (транспорт лежить); + // sent — дійшло принаймні одне. + sent, eligible := e.notifier.deliverEscalation(ctx, snap, d, byID) + switch { + case eligible == 0: e.logStep(ctx, snap, d, "no_channels", - "жоден канал сходинки не прийняв повідомлення (видалено, вимкнено або поріг серйозності)") - continue + "жоден канал сходинки не придатний: видалено, вимкнено або поріг серйозності вищий") + case sent == 0: + e.logStep(ctx, snap, d, "failed", + "жодне повідомлення сходинки не дійшло — подробиці в журналі доставки") + default: + // d.Outcome на останній сходинці — це 'done', і воно правильне + // для причини зупинки драбини, але не для рядка про доставку: + // підрахунок «скільки разів реально слали» інакше щоразу + // недорахував би останню сходинку кожного проходу. + e.logStep(ctx, snap, d, "sent", d.Detail) } - e.logStep(ctx, snap, d, d.Outcome, d.Detail) } } @@ -148,12 +166,11 @@ func (e *Engine) logStep(ctx context.Context, snap store.EscalationSnapshot, // Кількість потрібна тому, хто пише журнал: сходинка без жодного каналу // має лишити слід «нікуди не пішло», а не «надіслано». func (n *Notifier) deliverEscalation(ctx context.Context, snap store.EscalationSnapshot, - d store.EscalationDecision, byID map[string]store.Channel) int { + d store.EscalationDecision, byID map[string]store.Channel) (sent, eligible int) { a := snap.Alert head := escalationHeader(snap, d) - sent := 0 for _, id := range d.ChannelIDs { c, ok := byID[id] if !ok { @@ -167,16 +184,20 @@ func (n *Notifier) deliverEscalation(ctx context.Context, snap store.EscalationS if !c.Enabled || severityRank[a.Severity] < severityRank[c.MinSeverity] { continue } - n.sendText(ctx, snap.TenantID, a, c, head+renderMessage(a, c)) - sent++ + eligible++ + // Рахуємо доставки, а не спроби: сходинка, чиї повідомлення всі + // впали, не має лишати в журналі «надіслано». + if err := n.sendText(ctx, snap.TenantID, a, c, head+renderMessage(a, c)); err == nil { + sent++ + } } if sent == 0 { - n.log.Warn("сходинка ескалації не мала куди піти", + n.log.Warn("сходинка ескалації нікому не дійшла", "алерт", snap.AlertID, "сходинка", d.StepIdx+1, - "каналів у сходинці", len(d.ChannelIDs)) + "каналів у сходинці", len(d.ChannelIDs), "придатних", eligible) } - return sent + return sent, eligible } // escalationHeader пояснює людині, чому вона це читає. diff --git a/server/internal/alerting/notify.go b/server/internal/alerting/notify.go index 1ff2766..d53aa7a 100644 --- a/server/internal/alerting/notify.go +++ b/server/internal/alerting/notify.go @@ -136,8 +136,15 @@ func (n *Notifier) armEscalation(ctx context.Context, tenantID string, a store.A "tenant", tenantID, "правило", a.RuleName, "політика", act.EscalationPolicyID) return } + // Відлік драбини — від МОМЕНТУ ПЕРШОГО СПОВІЩЕННЯ, а не від початку + // алерту. Для щойно піднятого це той самий тік, різниці немає. А от + // для алерту, з якого щойно зняли заглушення, різниця вирішальна: + // відлік від started_at означав би, що вся драбина протухла ще у + // вікні обслуговування й висиплеться одним залпом. «П'ятнадцять + // хвилин на підтвердження» мають починатись тоді, коли людина вперше + // могла щось зробити. if err := n.st.ArmEscalation(ctx, tenantID, a.ID, p.ID, - store.IsEventSource(act.Source), p, a.StartedAt); err != nil { + store.IsEventSource(act.Source), p, time.Now()); err != nil { n.log.Error("взведення ескалації", "алерт", a.ID, "помилка", err) } } @@ -226,15 +233,23 @@ func (n *Notifier) targets(a store.Alert, act store.RuleAction, hasAct bool, } func (n *Notifier) send(ctx context.Context, tenantID string, a store.Alert, c store.Channel) { - n.sendText(ctx, tenantID, a, c, renderMessage(a, c)) + _ = n.sendText(ctx, tenantID, a, c, renderMessage(a, c)) } // sendText відокремлено від send, бо ескалація шле той самий алерт із // іншою шапкою: людина, розбуджена третьою сходинкою, має з першого // рядка бачити, що це вже ескалація, а не перше сповіщення, яке вона // проспала. +// sendText шле одне повідомлення й ПОВЕРТАЄ, чи дійшло. +// +// Повертає помилку не для того, щоб хтось її обробляв — журнал доставки +// пишеться тут же, і вище робити з нею нічого. Повертає, бо той, хто +// рахує «скільком дійшло», інакше рахує не доставки, а лише те, що +// повідомлення пройшло фільтр каналу. Різниця видна рівно тоді, коли +// вона дорога: Telegram лежить десять хвилин, усі доставки падають — а +// журнал ескалацій пише «надіслано». func (n *Notifier) sendText(ctx context.Context, tenantID string, a store.Alert, - c store.Channel, text string) { + c store.Channel, text string) error { var extID string var err error @@ -259,6 +274,7 @@ func (n *Notifier) sendText(ctx context.Context, tenantID string, a store.Alert, map[string]any{"text": text}); rerr != nil { n.log.Error("журнал доставки", "помилка", rerr) } + return err } // --------------------------------------------------------------------- diff --git a/server/internal/alerting/unsuppress_test.go b/server/internal/alerting/unsuppress_test.go new file mode 100644 index 0000000..8557b42 --- /dev/null +++ b/server/internal/alerting/unsuppress_test.go @@ -0,0 +1,30 @@ +package alerting + +import "testing" + +// Аварія, що почалась усередині вікна обслуговування, до цього не +// будила нікого й ніколи: сповіщення при народженні не йшло (заглушено), +// а коли вікно скінчилось — теж не йшло, бо алерт уже «не новий». Драбина +// ескалації при цьому теж не взводилась. +// +// Тест тримає обидва боки переходу: пропустити його — тиша на справжній +// аварії, спрацювати зайвий раз — другий дзвінок про те саме. +func TestLeftSuppression(t *testing.T) { + for _, c := range []struct { + prev, cur string + want bool + why string + }{ + {"suppressed", "firing", true, "вікно скінчилось, проблема лишилась — це перша звістка для людини"}, + {"firing", "suppressed", false, "пішло в заглушення: про це не сповіщають"}, + {"firing", "acknowledged", false, "хтось узяв у роботу"}, + {"suppressed", "resolved", false, "минулось саме — будити нема про що"}, + {"acknowledged", "firing", false, "зняли підтвердження: продовження відомої проблеми, не новина"}, + {"", "firing", false, "новий алерт іде своїм шляхом, не цим"}, + {"suppressed", "suppressed", false, "нічого не змінилось"}, + } { + if got := leftSuppression(c.prev, c.cur); got != c.want { + t.Errorf("%s → %s: маємо %v, очікували %v (%s)", c.prev, c.cur, got, c.want, c.why) + } + } +} diff --git a/server/internal/httpapi/alerts.go b/server/internal/httpapi/alerts.go index ba9160c..db6eef4 100644 --- a/server/internal/httpapi/alerts.go +++ b/server/internal/httpapi/alerts.go @@ -343,6 +343,15 @@ func (s *Server) handleCreateAlertRule(w http.ResponseWriter, r *http.Request, p writeError(w, http.StatusNotFound, "not_found", "правило не знайдено") return } + // Відмова від store — це відмова людині, а не збій сервера: + // саме тут виходить «правило шле в канал, якого немає в + // цьому кабінеті». Без цієї гілки перевірка спрацювала б, а + // назовні пішла б «внутрішня помилка» — тобто причини ніхто + // б не побачив. + if errors.Is(err, store.ErrInvalid) { + writeError(w, http.StatusBadRequest, "bad_channels", err.Error()) + return + } if isUniqueViolation(err) { writeError(w, http.StatusConflict, "duplicate", "правило з такою назвою вже є") return @@ -357,6 +366,10 @@ func (s *Server) handleCreateAlertRule(w http.ResponseWriter, r *http.Request, p id, err := s.store.CreateRule(r.Context(), p.TenantID, p.UserID, in) if err != nil { + if errors.Is(err, store.ErrInvalid) { + writeError(w, http.StatusBadRequest, "bad_channels", err.Error()) + return + } if isUniqueViolation(err) { writeError(w, http.StatusConflict, "duplicate", "правило з такою назвою вже є") return diff --git a/server/internal/store/alerts_channels.go b/server/internal/store/alerts_channels.go index 22fb1d9..22280ff 100644 --- a/server/internal/store/alerts_channels.go +++ b/server/internal/store/alerts_channels.go @@ -493,6 +493,40 @@ func (s *Store) DeleteChannel(ctx context.Context, tenantID, channelID string) e return err } } + + // Те саме й з тієї ж причини — для правил і маршрутів. + // + // alr.rules.channel_ids і alr.routes.channel_ids мають тип + // uuid[], а на масив зовнішнього ключа в Postgres немає, тож + // прибрати посилання нікому. Залишений UUID видаленого каналу + // коштує тут дорожче, ніж у сходинці: правило з явно вказаними + // каналами маршрутів уже не питає (див. targets), тож єдиний + // мертвий id перетворює правило на мовчазне — а нуль доставок + // не дає взвести й драбину. Правило, яке лишилось без каналів + // зовсім, повертається до загальних маршрутів кабінету — це + // його ж типовий стан, і він гучніший за порожню розсилку. + // + // Тут одним UPDATE, а не переписуванням у Go, як сходинки: із + // масивом рішення «що лишається» тривіальне (array_remove), і + // вигадувати для нього окрему функцію означало б зробити + // однакові речі різними. + if _, err := tx.Exec(ctx, ` + UPDATE alr.rules + SET channel_ids = array_remove(channel_ids, $2::uuid), updated_at = now() + WHERE tenant_id = $1 AND $2::uuid = ANY(channel_ids) + `, tenantID, channelID); err != nil { + return err + } + // Маршрути окремим оператором, бо в них немає updated_at: 0007 + // його не заводила, і додавати колонку заради симетрії двох + // UPDATE — не та ціна. + if _, err := tx.Exec(ctx, ` + UPDATE alr.routes + SET channel_ids = array_remove(channel_ids, $2::uuid) + WHERE tenant_id = $1 AND $2::uuid = ANY(channel_ids) + `, tenantID, channelID); err != nil { + return err + } return nil }) } diff --git a/server/internal/store/alerts_escalation.go b/server/internal/store/alerts_escalation.go index 0188aa2..130ccf4 100644 --- a/server/internal/store/alerts_escalation.go +++ b/server/internal/store/alerts_escalation.go @@ -286,12 +286,16 @@ const EscalationGrace = 24 * time.Hour // перезапускала б драбину з нуля: алерт висів би годинами, а «наступного» // будили б щоп'ятнадцять хвилин заново. func (s *Store) ArmEscalation(ctx context.Context, tenantID, alertID, policyID string, - isEvent bool, p EscalationPolicy, alertStarted time.Time) error { + isEvent bool, p EscalationPolicy, firstNotified time.Time) error { if len(p.Steps) == 0 { return nil } - passStart := alertStarted + // Відлік — від першого сповіщення, а не від початку алерту: див. + // armEscalation. Для алерту, з якого щойно зняли заглушення, це + // різниця між «драбина попереду» і «драбина протухла ще у вікні + // обслуговування й висиплеться одним залпом». + passStart := firstNotified next := passStart.Add(time.Duration(p.Steps[0].AfterMin) * time.Minute) deadline := passStart.Add(escalationSpan(p, isEvent)).Add(EscalationGrace) @@ -640,12 +644,29 @@ func (s *Store) TakeDueEscalations(ctx context.Context, limit int) ([]Escalation // записом: сходинка лишиться без рядка. Це видно (step_idx більший за // кількість рядків) і це чесно. // -// stopped_at IS NULL — не оптимізація, а перевірка стану. Між тим, як -// сходинку взяли в чергу, і тим, як до неї дійшли руки, людина могла -// натиснути «Прийняти»: партія обробляється послідовно, кожна доставка -// має власний таймаут, і розрив вимірюється хвилинами. Без цієї умови -// UPDATE зняв би stopped_at і воскресив зупинену драбину — тобто -// розбудив би саме того, хто щойно сказав «я цим займаюсь». +// ДВІ УМОВИ ПРО СТАН, і кожна закриває свій бік того самого розриву. +// Між тим, як сходинку взяли в чергу, і тим, як до неї дійшли руки, +// минають хвилини: партія обробляється послідовно, кожна доставка має +// власний таймаут. За цей час алерт міг перестати потребувати дзвінка. +// +// stopped_at IS NULL — людина натиснула «Прийняти» (або закрила алерт +// руками): ці шляхи ставлять stopped_at самі. Без умови UPDATE зняв би +// його й воскресив зупинену драбину — розбудив би саме того, хто +// щойно сказав «я цим займаюсь». +// +// алерт усе ще 'firing' (лише для сходинки, що має спрацювати) — +// решта шляхів гасіння рядка драбини НЕ чіпають: вимкнення чи +// видалення правила (resolveRuleAlerts), відновлення метрики +// (ResolveMissing), гасіння простроченого подієвого. Вони переводять +// алерт у 'resolved', і драбина зупиниться на НАСТУПНІЙ сходинці — а +// та, що вже в партії, без цієї умови подзвонить за погашеним +// алертом. Умова стоїть тут, а не в кожному з тих шляхів, саме тому +// що їх багато й з часом побільшає: перевіряти стан у момент дії +// надійніше, ніж пам'ятати про драбину в кожному новому місці. +// +// Якщо жодного рядка не оновлено, наступний такт візьме цю сходинку +// знову, PlanEscalation побачить 'resolved' і зупинить драбину штатно — +// тобто вона не застрягне. func (s *Store) ApplyEscalation(ctx context.Context, snap EscalationSnapshot, d EscalationDecision) (bool, error) { var next any if d.NextAt != nil { @@ -667,6 +688,10 @@ func (s *Store) ApplyEscalation(ctx context.Context, snap EscalationSnapshot, d stop_reason = CASE WHEN $5::timestamptz IS NULL THEN $7 ELSE NULL END WHERE alert_id = $1 AND stopped_at IS NULL + AND (NOT $6 OR EXISTS ( + SELECT 1 FROM alr.alerts a + WHERE a.id = alr.alert_escalations.alert_id + AND a.state = 'firing')) `, snap.AlertID, d.NextStepIdx, d.NextRepeatIdx, d.NextPassStart, next, fired, d.Outcome) if err != nil { @@ -687,13 +712,28 @@ func (s *Store) ApplyEscalation(ctx context.Context, snap EscalationSnapshot, d func (s *Store) LogEscalationStep(ctx context.Context, snap EscalationSnapshot, d EscalationDecision, outcome, detail string) error { + // Відкладання пишеться ОДИН раз на сходинку, а не щоперевірки. + // + // Заглушений алерт переглядається кожні EscalationRecheck, тобто + // приблизно 288 разів на добу, і драбина може чекати тижнями (стеля + // життя — до ~21 доби). Без цієї умови один алерт під вікном + // обслуговування лишав би тисячі однакових рядків, і журнал, у який + // заходять з питанням «чому мене не розбудили», перестав би бути + // придатним для читання саме тоді, коли він потрібен. Один рядок + // каже рівно те саме: цю сходинку відкладено через заглушення. + once := outcome == "suppressed" + return s.InTenantTx(ctx, snap.TenantID, func(tx pgx.Tx) error { _, err := tx.Exec(ctx, ` INSERT INTO alr.escalation_steps (tenant_id, alert_id, policy_id, step_idx, repeat_idx, outcome, detail) - VALUES ($1, $2, $3, $4, $5, $6, $7) + SELECT $1, $2, $3, $4, $5, $6, $7 + WHERE NOT $8 OR NOT EXISTS ( + SELECT 1 FROM alr.escalation_steps + WHERE alert_id = $2 AND step_idx = $4 + AND repeat_idx = $5 AND outcome = $6) `, snap.TenantID, snap.AlertID, nullUUID(snap.PolicyID), - d.StepIdx, d.RepeatIdx, outcome, nullString(detail)) + d.StepIdx, d.RepeatIdx, outcome, nullString(detail), once) return err }) } diff --git a/server/internal/store/alerts_escalation_db_test.go b/server/internal/store/alerts_escalation_db_test.go index ab26dc0..6cfa124 100644 --- a/server/internal/store/alerts_escalation_db_test.go +++ b/server/internal/store/alerts_escalation_db_test.go @@ -275,6 +275,82 @@ func TestEscalationAgainstDB(t *testing.T) { t.Errorf("зупинену драбину воскрешено: причина %q, наступна %v", raceStop, raceNext) } + // --- Гасіння правилом посеред партії теж не має дзвонити ------------ + // + // Той самий розрив, що й з ack, але шляхом, який рядка драбини НЕ + // чіпає: resolveRuleAlerts переводить алерт у 'resolved' і про + // драбину нічого не знає. Знімок узято ДО гасіння, рішення + // застосовується ПІСЛЯ. + a4 := newAlert(ruleID + ":dev:" + deviceID + ":4") + if err := st.ArmEscalation(ctx, tenantID, a4, policyID, false, policy, started); err != nil { + t.Fatal(err) + } + if _, err := st.pool.Exec(ctx, + `UPDATE alr.alert_escalations SET next_at = now() - interval '1 minute' WHERE alert_id = $1`, + a4); err != nil { + t.Fatal(err) + } + dueRule, err := st.TakeDueEscalations(ctx, 10) + if err != nil { + t.Fatal(err) + } + var snapRule EscalationSnapshot + for _, x := range dueRule { + if x.AlertID == a4 { + snapRule = x + } + } + if snapRule.AlertID == "" { + t.Fatal("сходинка не потрапила в чергу") + } + dRule := PlanEscalation(snapRule, time.Now()) + if dRule.Action != EscFire { + t.Fatalf("сходинка мала спрацювати: %v/%s", dRule.Action, dRule.Outcome) + } + // Гасимо так, як це робить вимкнення правила: рядок драбини не чіпаємо. + if _, err := st.pool.Exec(ctx, + `UPDATE alr.alerts SET state = 'resolved', resolved_at = now() WHERE id = $1`, + a4); err != nil { + t.Fatal(err) + } + appliedRule, err := st.ApplyEscalation(ctx, snapRule, dRule) + if err != nil { + t.Fatal(err) + } + if appliedRule { + t.Error("сходинка спрацювала за погашеним алертом — дзвінок ні про що") + } + // І драбина не застрягла: наступний такт зупинить її штатно. + if _, err := st.pool.Exec(ctx, + `UPDATE alr.alert_escalations SET leased_until = NULL WHERE alert_id = $1`, a4); err != nil { + t.Fatal(err) + } + againRule, err := st.TakeDueEscalations(ctx, 10) + if err != nil { + t.Fatal(err) + } + var back bool + for _, x := range againRule { + if x.AlertID != a4 { + continue + } + back = true + dd := PlanEscalation(x, time.Now()) + if dd.Action != EscStop || dd.Outcome != "closed" { + t.Errorf("наступний такт мав зупинити драбину: %v/%s", dd.Action, dd.Outcome) + } + okStop, err := st.ApplyEscalation(ctx, x, dd) + if err != nil { + t.Fatal(err) + } + if !okStop { + t.Error("зупинку не записано — драбина застрягла назавжди") + } + } + if !back { + t.Error("сходинка не повернулась у чергу — драбину нікому зупинити") + } + // --- Закритий алерт: сходинка не спрацьовує навіть якщо настала ---- a2 := newAlert(ruleID + ":dev:" + deviceID + ":2") if err := st.ArmEscalation(ctx, tenantID, a2, policyID, false, policy, started); err != nil { diff --git a/server/internal/store/alerts_query.go b/server/internal/store/alerts_query.go index 39f1b21..2cc8b4c 100644 --- a/server/internal/store/alerts_query.go +++ b/server/internal/store/alerts_query.go @@ -3,6 +3,7 @@ package store import ( "context" "errors" + "fmt" "time" "github.com/jackc/pgx/v5" @@ -436,9 +437,61 @@ func (s *Store) ListRules(ctx context.Context, tenantID string) ([]map[string]an return out, err } +// ValidateRuleChannels відмовляє в правилі, яке шле в канал, якого в +// цьому кабінеті немає. +// +// Та сама перевірка, що ValidateStepChannels робить для сходинок +// драбини, і зроблена навмисно так само: alr.rules.channel_ids має тип +// uuid[], а на масив зовнішнього ключа в Postgres немає — тобто в базу +// лягав би будь-який UUID, хоч вигаданий, хоч підставлений із сусіднього +// кабінету. +// +// Ціна помилки тут ВИЩА, ніж у драбині, і це головне. Сходинка з +// невідомим каналом глушить одну сходинку. Правило з невідомим каналом +// глушить усе: targets() (див. alerting/notify.go) для явно вказаних +// каналів правила маршрути вже не питає, жодного каналу не знаходить, +// sent лишається нулем — а через нуль shouldArmEscalation не взводить +// навіть драбину. Один переплутаний ідентифікатор вимикає і сповіщення, +// і резервний механізм, заведений рівно на випадок, коли сповіщення не +// спрацювало. +// +// Порожній перелік — не помилка й перевіряти його тут нічим: він +// означає «за загальними маршрутами кабінету» і є типовим станом +// правила. +// +// known — ідентифікатори каналів ЦЬОГО кабінету, прочитані під RLS у +// транзакції запису (tenantChannelIDs). Саме тому перевірка закриває й +// підстановку чужого UUID, не знаючи слова «чужий». +func ValidateRuleChannels(ids []string, known map[string]bool) error { + for _, id := range ids { + if known[id] { + continue + } + // Ідентифікатор у тексті лишаємо навмисно, з тієї ж причини, що + // й у сходинках: у формі канали обираються галочками, тож той, + // хто це побачив, шле правило не з форми — і має знати, який + // саме рядок не прийнято. + return fmt.Errorf("%w: правило шле в канал %s, якого немає в цьому кабінеті — "+ + "оберіть канал зі списку на сторінці «Канали»", ErrInvalid, id) + } + return nil +} + func (s *Store) CreateRule(ctx context.Context, tenantID, userID string, in RuleInput) (string, error) { var id string err := s.InTenantTx(ctx, tenantID, func(tx pgx.Tx) error { + // Перевірка тут, а не лише в HTTP: обробник — не єдиний шлях + // запису, і кожен наступний (імпорт, шаблон, API-токен) забув + // би її повторити. Показово, що в тому самому обробнику + // escalation_policy_id уже перевірявся на належність кабінету, а + // канали — ні. + known, err := tenantChannelIDs(ctx, tx, tenantID) + if err != nil { + return err + } + if err := ValidateRuleChannels(in.ChannelIDs, known); err != nil { + return err + } return tx.QueryRow(ctx, ` INSERT INTO alr.rules (tenant_id, name, description, source, severity, selector, condition, @@ -450,7 +503,7 @@ func (s *Store) CreateRule(ctx context.Context, tenantID, userID string, in Rule RETURNING id::text `, tenantID, in.Name, nullString(in.Description), in.Source, in.Severity, in.Selector, in.Condition, in.ForSeconds, in.DependsOnTopology, - in.Enabled == nil || *in.Enabled, nullUUID(userID), in.ChannelIDs, in.NotifySchedule, + in.Enabled == nil || *in.Enabled, nullUUID(userID), uuidArray(in.ChannelIDs), in.NotifySchedule, in.NotifyOnResolve, in.AutoCloseSeconds, in.MinIntervalSeconds, nullUUID(in.EscalationPolicyID)).Scan(&id) }) @@ -519,6 +572,24 @@ func resolveRuleAlerts(ctx context.Context, tx pgx.Tx, tenantID, ruleID, reason return err } +// uuidArray готує перелік каналів до запису. +// +// Стовпець `channel_ids uuid[] NOT NULL DEFAULT '{}'`, і саме через +// DEFAULT здається, що з порожнім переліком усе гаразд. Але DEFAULT +// спрацьовує лише на ВІДСУТНІЙ стовпець; nil-зріз із Go їде як явний +// NULL і валить запис. Правило без власних каналів — типовий стан +// («сповіщати за загальними маршрутами кабінету»), тож ця невідповідність +// означала 500 на цілком звичайній дії. +// +// Знайдено прогоном проти справжньої бази; жоден тест на структурах +// цього побачити не міг. +func uuidArray(ids []string) []string { + if ids == nil { + return []string{} + } + return ids +} + func rawJSON(s string) any { if s == "" { return nil @@ -581,6 +652,21 @@ func (s *Store) UpdateRule(ctx context.Context, tenantID, ruleID string, in Rule } enabled, resolve := planRuleEnabled(was, in.Enabled) + // Канали перевіряємо ПІСЛЯ того, як переконались, що правило + // існує: інакше правка чужого правила з поганим каналом сказала + // б «немає такого каналу» замість «немає такого правила», тобто + // відповіла б не на те питання, яке поставили. + // + // Перелік читається в цій же транзакції — між перевіркою й + // записом не лишається щілини, в якій канал встигає зникнути. + known, err := tenantChannelIDs(ctx, tx, tenantID) + if err != nil { + return err + } + if err := ValidateRuleChannels(in.ChannelIDs, known); err != nil { + return err + } + ct, err := tx.Exec(ctx, ` UPDATE alr.rules SET name = $3, description = $4, source = $5::alr.rule_source, @@ -597,7 +683,7 @@ func (s *Store) UpdateRule(ctx context.Context, tenantID, ruleID string, in Rule WHERE id = $1 AND tenant_id = $2 `, ruleID, tenantID, in.Name, nullString(in.Description), in.Source, in.Severity, in.Selector, in.Condition, in.ForSeconds, - in.DependsOnTopology, enabled, in.ChannelIDs, in.NotifySchedule, + in.DependsOnTopology, enabled, uuidArray(in.ChannelIDs), in.NotifySchedule, in.NotifyOnResolve, in.AutoCloseSeconds, in.MinIntervalSeconds, nullUUID(in.EscalationPolicyID)) if err != nil { diff --git a/server/internal/store/alerts_rule_channels_db_test.go b/server/internal/store/alerts_rule_channels_db_test.go new file mode 100644 index 0000000..979cef5 --- /dev/null +++ b/server/internal/store/alerts_rule_channels_db_test.go @@ -0,0 +1,220 @@ +package store + +import ( + "context" + "errors" + "os" + "strings" + "testing" + "time" +) + +// Канали правил і маршрутів ПРОТИ БАЗИ. +// +// Чиста ValidateRuleChannels перевіряє рішення, і це половина. Друга +// половина існує тільки в базі й рішенням не перевіряється взагалі: +// +// - чи справді перелік каналів, який отримує перевірка, звужений до +// кабінету (тобто чи не проходить UUID сусіда через RLS); +// - чи стоїть перевірка на ОБОХ шляхах запису — і на створенні, і на +// правці; +// - чи прибирає видалення каналу посилання на нього з uuid[]-списків +// правил і маршрутів: зовнішнього ключа на масив немає, тож +// ON DELETE SET NULL, яким 0066 чистить драбину з правила, тут не +// спрацює. +// +// Мовчки пропускається без NETPULSE_TEST_DSN. Запускати треба на +// ОДНОРАЗОВІЙ базі — тест створює два кабінети й видаляє їх з усім +// вмістом (scripts/dbtest.sh робить це сам): +// +// NETPULSE_TEST_DSN=postgres://postgres:x@localhost/np \ +// go test ./internal/store/ -run RuleChannelsAgainstDB -v +func TestRuleChannelsAgainstDB(t *testing.T) { + dsn := os.Getenv("NETPULSE_TEST_DSN") + if dsn == "" { + t.Skip("NETPULSE_TEST_DSN не задано — перевірка проти бази пропускається") + } + ctx := context.Background() + + st, err := New(ctx, dsn) + if err != nil { + t.Fatalf("підключення: %v", err) + } + t.Cleanup(st.Close) + + stamp := strings.ReplaceAll(time.Now().Format("150405.000"), ".", "") + + newTenant := func(prefix string) string { + t.Helper() + var id string + if err := st.pool.QueryRow(ctx, ` + INSERT INTO core.tenants (slug, name) VALUES ($1, $2) RETURNING id::text + `, prefix+"-"+stamp, "Перевірка каналів правил").Scan(&id); err != nil { + t.Fatalf("кабінет %s: %v", prefix, err) + } + t.Cleanup(func() { + _, _ = st.pool.Exec(context.Background(), `DELETE FROM core.tenants WHERE id = $1`, id) + }) + return id + } + + newChannel := func(tenantID, name string) string { + t.Helper() + var id string + if err := st.pool.QueryRow(ctx, ` + INSERT INTO alr.channels (tenant_id, kind, name, config, min_severity, enabled) + VALUES ($1, 'webhook', $2, '{}'::jsonb, 'warning', true) RETURNING id::text + `, tenantID, name).Scan(&id); err != nil { + t.Fatalf("канал %s: %v", name, err) + } + return id + } + + ours := newTenant("rulech-a") + theirs := newTenant("rulech-b") + + duty := newChannel(ours, "черговий") + lead := newChannel(ours, "старший зміни") + alien := newChannel(theirs, "чужий канал") + + rule := func(name string, channels ...string) RuleInput { + return RuleInput{ + Name: name, + Source: "icmp", + Severity: "high", + Selector: "{}", + Condition: `{"metric":"loss_pct","op":">","value":20}`, + ForSeconds: 60, + ChannelIDs: channels, + } + } + // Канали правила читаємо тим самим шляхом, яким їх читає движок + // доставки: перевіряти сирим SELECT означало б перевірити колонку, а + // не те, що справді потрапляє в targets(). + ruleChannels := func(tenantID, ruleID string) []string { + t.Helper() + acts, err := st.LoadRuleActions(ctx, tenantID) + if err != nil { + t.Fatalf("маршрутизація правил: %v", err) + } + return acts[ruleID].ChannelIDs + } + + // --- Канал ЧУЖОГО кабінету не зберігається ------------------------- + // + // Найдорожчий випадок. Ззовні це не витік — доставити в чужий канал + // усе одно нікуди, движок читає канали лише свого кабінету, — а + // гірше: правило виглядає налаштованим і мовчить, і разом із ним + // мовчить драбина, бо sent == 0. Закривати це в HTTP марно: обійде + // будь-який інший шлях запису, тому тест б'є прямо в store. + _, err = st.CreateRule(ctx, ours, "", rule("Підсунутий сусід", alien)) + if err == nil { + t.Fatal("правило з каналом ЧУЖОГО кабінету збереглося") + } + if !errors.Is(err, ErrInvalid) { + t.Fatalf("відмова має бути ErrInvalid: %v", err) + } + if !strings.Contains(err.Error(), alien) { + t.Fatalf("відмова не називає підставлений ідентифікатор: %v", err) + } + + // --- Неіснуючий канал не зберігається ------------------------------ + const ghost = "00000000-0000-4000-8000-0000000000fe" + if _, err := st.CreateRule(ctx, ours, "", rule("Привид", duty, ghost)); err == nil { + t.Fatal("правило з неіснуючим каналом збереглося") + } else if !strings.Contains(err.Error(), ghost) { + t.Fatalf("відмова не називає неіснуючий канал: %v", err) + } + + // --- Справне правило зберігається ---------------------------------- + ruleID, err := st.CreateRule(ctx, ours, "", rule("Ядро не відповідає", duty, lead)) + if err != nil { + t.Fatalf("справне правило не збереглося: %v", err) + } + + // --- Правило без каналів теж зберігається -------------------------- + // + // Порожньо означає «за загальними маршрутами кабінету» і є типовим + // станом. Перевірка, яка ловить і це, зламала б усі наявні правила. + if _, err := st.CreateRule(ctx, ours, "", rule("Без каналів")); err != nil { + t.Fatalf("правило без каналів відхилено: %v", err) + } + + // --- Правка ЗАКРИТА так само, як створення ------------------------- + // + // Окремий випадок навмисно: перевірка лише на створенні лишала б + // двері відчиненими — правило заводять справним, а ламають правкою. + upd := rule("Ядро не відповідає", duty, alien) + if err := st.UpdateRule(ctx, ours, ruleID, upd); err == nil { + t.Fatal("правка підставила канал чужого кабінету") + } else if !errors.Is(err, ErrInvalid) { + t.Fatalf("відмова правки має бути ErrInvalid: %v", err) + } + // І правило має лишитись таким, яким було: відкинута правка не має + // залишати по собі половину змін. + if got := ruleChannels(ours, ruleID); len(got) != 2 { + t.Fatalf("відкинута правка змінила канали правила: %v", got) + } + + // --- «Немає такого правила» важливіше за «немає такого каналу» ----- + // + // Порядок перевірок відповідає на те питання, яке поставили: правка + // чужого (або видаленого) правила має казати «правило не знайдено», + // а не звітувати про стан каналів у ньому. + err = st.UpdateRule(ctx, ours, "00000000-0000-4000-8000-0000000000fd", upd) + if !errors.Is(err, ErrNotFound) { + t.Fatalf("правка неіснуючого правила має давати ErrNotFound: %v", err) + } + + // --- Маршрут із каналами ------------------------------------------- + // + // Маршрути заводяться прямим SQL, бо шляху запису для них у коді + // немає взагалі (див. звіт): є лише LoadRoutes. Тест перевіряє те + // єдине, що з ними може статись, — видалення каналу. + var routeID string + if err := st.pool.QueryRow(ctx, ` + INSERT INTO alr.routes (tenant_id, name, priority, matcher, channel_ids, enabled) + VALUES ($1, $2, 100, '{}'::jsonb, ARRAY[$3::uuid, $4::uuid], true) + RETURNING id::text + `, ours, "нічне чергування "+stamp, duty, lead).Scan(&routeID); err != nil { + t.Fatalf("маршрут: %v", err) + } + + // --- Видалення каналу чистить обидва списки ------------------------ + // + // Симетрично сходинкам драбини: залишений UUID видаленого каналу — + // це правило, яке виглядає налаштованим і не шле нікуди. + if err := st.DeleteChannel(ctx, ours, duty); err != nil { + t.Fatalf("видалення каналу: %v", err) + } + got := ruleChannels(ours, ruleID) + if len(got) != 1 || got[0] != lead { + t.Fatalf("правило після видалення каналу: %v (мав лишитись лише старший зміни)", got) + } + routes, err := st.LoadRoutes(ctx, ours) + if err != nil { + t.Fatalf("маршрути: %v", err) + } + var route Route + for _, r := range routes { + if r.ID == routeID { + route = r + } + } + if len(route.ChannelIDs) != 1 || route.ChannelIDs[0] != lead { + t.Fatalf("маршрут після видалення каналу: %v", route.ChannelIDs) + } + + // --- Правило, вичищене до порожнечі, лишається справним ------------ + // + // Після видалення другого каналу правило повертається до загальних + // маршрутів кабінету. Це його ж типовий стан і він гучніший за + // порожню розсилку — тому канали саме прибираються, а не лишаються + // мертвими. + if err := st.DeleteChannel(ctx, ours, lead); err != nil { + t.Fatalf("видалення другого каналу: %v", err) + } + if got := ruleChannels(ours, ruleID); len(got) != 0 { + t.Fatalf("правило лишилось із посиланнями на видалені канали: %v", got) + } +} diff --git a/server/internal/store/alerts_rule_channels_test.go b/server/internal/store/alerts_rule_channels_test.go new file mode 100644 index 0000000..e77b85b --- /dev/null +++ b/server/internal/store/alerts_rule_channels_test.go @@ -0,0 +1,93 @@ +package store + +import ( + "errors" + "strings" + "testing" +) + +// Перевірки правила, яке шле в канал. +// +// Та сама вада, що вже закрита для сходинок драбини, тільки дорожча. +// alr.rules.channel_ids має тип uuid[], зовнішнього ключа на масив у +// Postgres немає — тож у правило лягав будь-який UUID: хоч вигаданий, +// хоч підставлений із сусіднього кабінету. У формі таке правило +// виглядало налаштованим на канал. +// +// Ціна: targets() для правила з явно вказаними каналами маршрути вже не +// питає, жодного каналу за мертвим id не знаходить, sent лишається +// нулем — і через нуль shouldArmEscalation не взводить навіть драбину. +// Тобто одна помилка в ідентифікаторі глушить і сповіщення, і резервний +// механізм, заведений рівно на випадок, коли сповіщення не спрацювало. + +// Головний випадок: канал, якого в кабінеті немає. +func TestRuleChannelUnknownRefused(t *testing.T) { + err := ValidateRuleChannels([]string{"ch-duty", "ch-gone"}, knownChannels("ch-duty", "ch-lead")) + if err == nil { + t.Fatal("правило з неіснуючим каналом збереглося — воно нікого не сповістить") + } + if !errors.Is(err, ErrInvalid) { + t.Fatalf("відмова має бути ErrInvalid (інакше HTTP віддасть 500): %v", err) + } + // Без ідентифікатора у відмові людина не знає, який саме рядок + // виправляти: у переліку каналів правила їх буває кілька. + if !strings.Contains(err.Error(), "ch-gone") { + t.Fatalf("відмова не називає ідентифікатор: %q", err) + } + // І де взяти правильний. Відмова, яка лише каже «ні», змушує людину + // здогадуватись — саме цього уникає формулювання сходинок драбини. + if !strings.Contains(err.Error(), "Канали") { + t.Fatalf("відмова не підказує, де взяти правильний канал: %q", err) + } +} + +// Дзеркальний випадок: усе на місці — відмови бути не має. +func TestRuleChannelsKnownAccepted(t *testing.T) { + if err := ValidateRuleChannels([]string{"ch-duty", "ch-lead"}, + knownChannels("ch-duty", "ch-lead", "ch-boss")); err != nil { + t.Fatalf("справне правило не збереглося: %v", err) + } +} + +// Канал чужого кабінету — той самий випадок, і це головне в ньому. +// +// Перевірка не знає слова «чужий»: їй дають перелік каналів ЦЬОГО +// кабінету, прочитаний під RLS у транзакції запису. Тому підставлений +// UUID сусіда не проходить не як окремий випадок, а як частина +// загального правила — і його не можна забути закрити окремо. +func TestRuleChannelFromOtherTenantRefused(t *testing.T) { + const foreign = "00000000-0000-4000-8000-000000000002" + err := ValidateRuleChannels([]string{foreign}, knownChannels("ch-duty")) + if err == nil { + t.Fatal("канал чужого кабінету прийнято в правило") + } + if !strings.Contains(err.Error(), foreign) { + t.Fatalf("відмова не називає ідентифікатор: %q", err) + } +} + +// Порожній перелік — не помилка, а типовий стан правила. +// +// Порожньо означає «за загальними маршрутами кабінету»; відмовляти тут +// означало б вимагати канал у кожного правила, тобто зламати всі наявні +// однією перевіркою. +func TestRuleChannelsEmptyAllowed(t *testing.T) { + if err := ValidateRuleChannels(nil, knownChannels()); err != nil { + t.Fatalf("правило без каналів відхилено: %v", err) + } + if err := ValidateRuleChannels([]string{}, knownChannels("ch-duty")); err != nil { + t.Fatalf("порожній перелік каналів відхилено: %v", err) + } +} + +// Вимкнений канал — не привід відмовляти, як і в драбині. +// +// Перелік каналів кабінету не фільтрується за enabled навмисно: +// «вимкнув Telegram на час переїзду» не має ламати збереження правила, +// у якому він стоїть. Про вимкнений канал говорить форма, і це інша +// розмова, ніж «такого каналу немає». +func TestRuleChannelDisabledStillValid(t *testing.T) { + if err := ValidateRuleChannels([]string{"ch-off"}, knownChannels("ch-off")); err != nil { + t.Fatalf("вимкнений канал відхилено: %v", err) + } +}