From d1aff44ca5b3105b29031e4afef2e08d34990d2a Mon Sep 17 00:00:00 2001 From: Loakesh Indiran Date: Tue, 3 Nov 2026 15:15:53 +0530 Subject: [PATCH] fix(oncall): stage 2 fix, review pass 3 A platform skip is no longer counted as a delivery, so a test page or a notice that only reaches PagerDuty, Opsgenie or ServiceNow reports that nothing was sent. The policy handler checks the template first, writes the policy, and only then writes and replicates the template override, so a refused policy leaves no half-applied override. The upgrade line also scans the team's own channel destinations for email and SNS. --- src/api/management/src/request/oncall/mod.rs | 44 +++++++++++--------- src/core/src/alerts/notifications/oncall.rs | 24 +++++++---- 2 files changed, 41 insertions(+), 27 deletions(-) diff --git a/src/api/management/src/request/oncall/mod.rs b/src/api/management/src/request/oncall/mod.rs index 6e8543f908..d108b623b5 100644 --- a/src/api/management/src/request/oncall/mod.rs +++ b/src/api/management/src/request/oncall/mod.rs @@ -1674,29 +1674,22 @@ pub async fn set_policy( other => internal_error("set_policy node versions", &other), }; } - if let Some(template) = body.template.clone() { - let template = template - .map(|t| t.trim().to_string()) - .filter(|t| !t.is_empty()); - if let Some(name) = template.as_deref() - && let Err(e) = openobserve_core::alerts::templates::check_binding( - &org_id, - name, - config::meta::alerts::notification_kind::NotificationKind::OnCall, - ) - .await - { - return MetaHttpResponse::bad_request(e.as_str()); - } - if let Err(e) = o2_enterprise::enterprise::oncall::service::set_policy_template( - &org_id, &team_id, template, + // Checked before any write, written after the policy, so a refused policy leaves no half. + let template = body + .template + .clone() + .map(|t| t.map(|t| t.trim().to_string()).filter(|t| !t.is_empty())); + if let Some(Some(name)) = template.as_ref() + && let Err(e) = openobserve_core::alerts::templates::check_binding( + &org_id, + name, + config::meta::alerts::notification_kind::NotificationKind::OnCall, ) .await - { - return to_response(e); - } + { + return MetaHttpResponse::bad_request(e.as_str()); } - match o2_enterprise::enterprise::oncall::service::set_policy( + let policy = match o2_enterprise::enterprise::oncall::service::set_policy( &org_id, &team_id, body.rungs, @@ -1704,6 +1697,17 @@ pub async fn set_policy( body.l0, ) .await + { + Ok(policy) => policy, + Err(e) => return to_response(e), + }; + let Some(template) = template else { + return MetaHttpResponse::json(policy); + }; + match o2_enterprise::enterprise::oncall::service::set_policy_template( + &org_id, &team_id, template, + ) + .await { Ok(policy) => MetaHttpResponse::json(policy), Err(e) => to_response(e), diff --git a/src/core/src/alerts/notifications/oncall.rs b/src/core/src/alerts/notifications/oncall.rs index 8414beaff8..812dd7d368 100644 --- a/src/core/src/alerts/notifications/oncall.rs +++ b/src/core/src/alerts/notifications/oncall.rs @@ -20,7 +20,7 @@ use o2_enterprise::enterprise::oncall::sender::{OnCallNotification, OnCallSendEr use super::{ LegacyPayload, NotificationContext, STATUS_FIRING, - notify::{NotifyError, SkipReason, notify}, + notify::{NotifyError, notify}, }; /// Sends one on-call message to one destination through the shared path. @@ -44,7 +44,16 @@ pub async fn log_mail_destination_policies() { }; for policy in policies { let mut mail = Vec::new(); - for name in &policy.destinations { + // The team's own channel pages too, so its destinations count as well (D9). + let channel = infra::table::oncall_teams::get_channel(&policy.org_id, &policy.team_id) + .await + .ok() + .flatten() + .unwrap_or_default(); + let mut names: Vec<&String> = policy.destinations.iter().chain(channel.iter()).collect(); + names.sort(); + names.dedup(); + for name in names { if let Ok(Some(dest)) = infra::table::destinations::get(&policy.org_id, name).await && let Module::Alert { destination_type: DestinationType::Email(_) | DestinationType::Sns(_), @@ -119,7 +128,8 @@ pub fn on_call_context(n: &OnCallNotification) -> NotificationContext { /// A Retryable error is retried by the engine; a platform skip is neither delivered nor failed. fn send_result(result: Result<(), NotifyError>) -> Result<(), OnCallSendError> { match result { - Ok(()) | Err(NotifyError::Skipped(SkipReason::PlatformNotForKind)) => Ok(()), + Ok(()) => Ok(()), + // A skip is not a delivery, so a send that only reached platforms reports nothing sent. Err(NotifyError::Skipped(reason)) => Err(OnCallSendError::Skipped(reason.as_str().into())), Err(e) if e.is_retryable() => Err(OnCallSendError::Retryable(e.to_string())), Err(e) => Err(OnCallSendError::Permanent(e.to_string())), @@ -132,7 +142,7 @@ mod tests { use super::*; use crate::alerts::notifications::{ - notify::{Route, route}, + notify::{Route, SkipReason, route}, org_default::{EffectiveTemplate, TemplateSource, compiled_fallback_for}, }; @@ -245,10 +255,10 @@ mod tests { #[test] fn send_results_map_to_what_the_engine_retries() { assert_eq!(send_result(Ok(())), Ok(())); - assert_eq!( + assert!(matches!( send_result(Err(NotifyError::Skipped(SkipReason::PlatformNotForKind))), - Ok(()) - ); + Err(OnCallSendError::Skipped(_)) + )); assert!(matches!( send_result(Err(NotifyError::Skipped(SkipReason::NoOrgMemberRecipient))), Err(OnCallSendError::Skipped(_))