Skip to content

Commit 2a1565d

Browse files
committed
fix(checks): purge the remaining check-name-only keying
Surveyed every surface that correlated checks by bare name and scoped each to the (source, check) identity: - The snapshot rollup and its silenced-checks set now consult only the status row's own source (a status's checks all belong to one source); silenced_health_checks_for_server takes the source and the unused batch variant and silenced_refs_with_prefix are gone. - The device-facing effective check map only overlays the requested source's own silences — a silence on another source's same-named check no longer tells this source to skip it. - The server-detail checks table builds its skip-style set from the status source's silences only (the chips were already exact). - The rule-editor sample takes (source, check) so another source's same-named payload can't seed the editor's variables. - Issue::list_by_ref (the any-source ref lookup) lost its last caller with the attention rekey and is deleted.
1 parent f2e425d commit 2a1565d

13 files changed

Lines changed: 131 additions & 196 deletions

File tree

crates/database/src/issues.rs

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2092,26 +2092,6 @@ impl Issue {
20922092
.collect())
20932093
}
20942094

2095-
/// Like [`Self::list_by_source_ref`], but matching the ref under any
2096-
/// source. Healthcheck refs (`health/<check>`) are keyed per reporting
2097-
/// source; consumers that correlate by check name alone use this.
2098-
pub async fn list_by_ref(
2099-
db: &mut AsyncPgConnection,
2100-
ref_: &str,
2101-
server_ids: &[Uuid],
2102-
) -> Result<Vec<Self>> {
2103-
use crate::schema::issues::dsl;
2104-
if server_ids.is_empty() {
2105-
return Ok(Vec::new());
2106-
}
2107-
dsl::issues
2108-
.select(Self::as_select())
2109-
.filter(dsl::ref_.eq(ref_).and(dsl::server_id.eq_any(server_ids)))
2110-
.load(db)
2111-
.await
2112-
.map_err(AppError::from)
2113-
}
2114-
21152095
/// Mark an issue as operator-resolved. Triggers incident-membership
21162096
/// re-evaluation (typically: leaves the incident).
21172097
pub async fn resolve(

crates/database/src/silenced_refs.rs

Lines changed: 22 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
//! source-reported checks, while the scoped-policy storage is keyed by
1313
//! bare check name. The mapping is applied on the way in and out.
1414
15-
use std::collections::{BTreeSet, HashMap};
15+
use std::collections::BTreeSet;
1616

1717
use commons_errors::Result;
1818
use diesel::prelude::*;
@@ -111,110 +111,37 @@ pub async fn is_silenced(
111111
))
112112
}
113113

114-
/// All refs silenced for this server under any source and starting with
115-
/// `ref_prefix`, combining the server's own silences with its group's.
116-
/// `group_id` is the server's current group; pass `None` if the server is
117-
/// ungrouped. Used to build the device-facing effective check-severity map.
118-
/// May contain duplicates when a ref is silenced at both scopes.
119-
pub async fn silenced_refs_with_prefix(
114+
/// Check names silenced for a server under one reporting source, at
115+
/// either server or group scope. `group_id` is the server's current
116+
/// group; pass `None` if ungrouped. A check's identity is the (source,
117+
/// check) pair, so a silence on another source's same-named check never
118+
/// applies.
119+
///
120+
/// This feeds [`crate::statuses::Status::health_state_ignoring`] (a
121+
/// status row's checks all belong to the row's one source) and the
122+
/// device-facing effective check map: a silenced check keeps recording
123+
/// results but is presented as skipped and doesn't count toward the
124+
/// server's health rollup.
125+
pub async fn silenced_health_checks_for_server(
120126
db: &mut AsyncPgConnection,
121127
server_id: Uuid,
122128
group_id: Option<Uuid>,
123-
ref_prefix: &str,
124-
) -> Result<Vec<String>> {
125-
let mut refs: Vec<String> =
126-
ScopedCheckPolicy::list_silences(db, PolicyScope::Server(server_id))
127-
.await?
128-
.into_iter()
129-
.map(|p| check_to_ref(&p.source, &p.check_name))
130-
.collect();
131-
if let Some(gid) = group_id {
132-
refs.extend(
133-
ScopedCheckPolicy::list_silences(db, PolicyScope::Group(gid))
134-
.await?
135-
.into_iter()
136-
.map(|p| check_to_ref(&p.source, &p.check_name)),
137-
);
138-
}
139-
refs.retain(|r| r.starts_with(ref_prefix));
140-
Ok(refs)
141-
}
142-
143-
/// Healthcheck names silenced for each of the given `(server, group)`
144-
/// pairs, at either scope, whichever source they were silenced under.
145-
/// Pass each server's current group id (`None` for ungrouped). One batch
146-
/// query regardless of how many servers are asked about. Servers with no
147-
/// applicable silences are absent from the map.
148-
///
149-
/// This feeds [`crate::statuses::Status::health_state_ignoring`]: a
150-
/// silenced check keeps recording results but is presented as skipped
151-
/// and doesn't count toward the server's health rollup.
152-
pub async fn silenced_health_checks_for_servers(
153-
db: &mut AsyncPgConnection,
154-
servers: &[(Uuid, Option<Uuid>)],
155-
) -> Result<HashMap<Uuid, BTreeSet<String>>> {
129+
source: &str,
130+
) -> Result<BTreeSet<String>> {
156131
use crate::schema::scoped_check_policies::dsl;
157132

158-
let mut out: HashMap<Uuid, BTreeSet<String>> = HashMap::new();
159-
if servers.is_empty() {
160-
return Ok(out);
161-
}
162-
163-
let server_ids: Vec<Uuid> = servers.iter().map(|(id, _)| *id).collect();
164-
let group_ids: Vec<Uuid> = servers
165-
.iter()
166-
.filter_map(|(_, group)| *group)
167-
.collect::<BTreeSet<_>>()
168-
.into_iter()
169-
.collect();
170-
171-
// Health rollups match by check name across every reporting source;
172-
// canopy/manual silences aren't source-reported checks and don't
173-
// belong in the ignore-set.
174-
let rows: Vec<(Option<Uuid>, Option<Uuid>, String)> = dsl::scoped_check_policies
175-
.select((dsl::server_id, dsl::server_group_id, dsl::check_name))
133+
let rows: Vec<String> = dsl::scoped_check_policies
134+
.select(dsl::check_name)
176135
.filter(dsl::ceiling.eq("skipped"))
177-
.filter(dsl::source.ne_all([CANOPY_SOURCE, MANUAL_SOURCE]))
136+
.filter(dsl::source.eq(source))
178137
.filter(
179-
dsl::server_id
180-
.eq_any(&server_ids)
181-
.or(dsl::server_group_id.eq_any(&group_ids)),
138+
dsl::server_id.eq(server_id).or(dsl::server_group_id
139+
.is_not_distinct_from(group_id)
140+
.and(dsl::server_group_id.is_not_null())),
182141
)
183142
.load(db)
184143
.await?;
185-
186-
let mut by_group: HashMap<Uuid, Vec<String>> = HashMap::new();
187-
for (server_id, group_id, check) in rows {
188-
if let Some(sid) = server_id {
189-
out.entry(sid).or_default().insert(check);
190-
} else if let Some(gid) = group_id {
191-
by_group.entry(gid).or_default().push(check);
192-
}
193-
}
194-
for (server_id, group_id) in servers {
195-
if let Some(checks) = group_id.as_ref().and_then(|g| by_group.get(g)) {
196-
out.entry(*server_id)
197-
.or_default()
198-
.extend(checks.iter().cloned());
199-
}
200-
}
201-
202-
Ok(out)
203-
}
204-
205-
/// Single-server variant of [`silenced_health_checks_for_servers`].
206-
/// `group_id` is the server's current group; pass `None` if ungrouped.
207-
pub async fn silenced_health_checks_for_server(
208-
db: &mut AsyncPgConnection,
209-
server_id: Uuid,
210-
group_id: Option<Uuid>,
211-
) -> Result<BTreeSet<String>> {
212-
Ok(
213-
silenced_health_checks_for_servers(db, &[(server_id, group_id)])
214-
.await?
215-
.remove(&server_id)
216-
.unwrap_or_default(),
217-
)
144+
Ok(rows.into_iter().collect())
218145
}
219146

220147
impl ServerSilencedRef {

crates/database/src/statuses.rs

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -454,13 +454,16 @@ impl Status {
454454
Ok(filed)
455455
}
456456

457-
/// Most recent status row (across all servers) whose `health` array
458-
/// contains an entry for `check_name`. Used by the rule-editor UI to
459-
/// surface a realistic sample of the variables an operator can
460-
/// predicate on (the check's extras, the status-level extras, and
461-
/// the server's tags resolved up the group).
457+
/// Most recent status row (across all servers) pushed by `source`
458+
/// whose `health` array contains an entry for `check_name`. Used by
459+
/// the rule-editor UI to surface a realistic sample of the variables
460+
/// an operator can predicate on (the check's extras, the status-level
461+
/// extras, and the server's tags resolved up the group). Scoped to
462+
/// the check's own source — another source's same-named check may
463+
/// carry entirely different fields.
462464
pub async fn latest_for_check_name(
463465
db: &mut AsyncPgConnection,
466+
source: &str,
464467
check_name: &str,
465468
) -> Result<Option<Status>> {
466469
use diesel::sql_types::{Text, Uuid as DUuid};
@@ -477,9 +480,11 @@ impl Status {
477480
// express cleanly), then load the typed Status row by id.
478481
let picked: Option<Picked> = sql_query(
479482
"SELECT id FROM statuses \
480-
WHERE health @> jsonb_build_array(jsonb_build_object('check', $1::text)) \
483+
WHERE source = $1 \
484+
AND health @> jsonb_build_array(jsonb_build_object('check', $2::text)) \
481485
ORDER BY created_at DESC LIMIT 1",
482486
)
487+
.bind::<Text, _>(source)
483488
.bind::<Text, _>(check_name)
484489
.get_result(db)
485490
.await

crates/database/tests/it/check_severity_map.rs

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
//! Queries backing the device-facing effective check map:
22
//! `CheckPolicy::ceiling_map_for_source` (static policy ceilings,
3-
//! ignoring conditional rules) and `silenced_refs::silenced_refs_with_prefix`
4-
//! (server- plus group-scope silences under a ref prefix).
3+
//! ignoring conditional rules) and
4+
//! `silenced_refs::silenced_health_checks_for_server` (server- plus
5+
//! group-scope silences under one reporting source).
56
67
use commons_types::status::CheckResult;
78
use database::check_policies::{CheckPolicy, IfLadder};
89
use database::silenced_refs::{
9-
ServerGroupSilencedRef, ServerSilencedRef, silenced_refs_with_prefix,
10+
ServerGroupSilencedRef, ServerSilencedRef, silenced_health_checks_for_server,
1011
};
1112
use diesel::{sql_query, sql_types};
1213
use diesel_async::RunQueryDsl;
@@ -107,7 +108,7 @@ async fn ceiling_map_ignores_conditional_rules() {
107108
}
108109

109110
#[tokio::test(flavor = "multi_thread")]
110-
async fn silenced_refs_with_prefix_combines_scopes_and_filters() {
111+
async fn silenced_checks_combine_scopes_and_stay_per_source() {
111112
commons_tests::db::TestDb::run(async |mut conn, _| {
112113
let group_id = insert_group(&mut conn).await;
113114
let server_id = insert_server(&mut conn, Some(group_id)).await;
@@ -119,8 +120,9 @@ async fn silenced_refs_with_prefix_combines_scopes_and_filters() {
119120
ServerGroupSilencedRef::add(&mut conn, group_id, "alertd", "health/groupwide", None)
120121
.await
121122
.expect("group silence");
122-
// A silence under any source matches: healthcheck refs are keyed
123-
// per reporting source, and this helper correlates by check name.
123+
// None of these may leak into alertd's set: a check's identity is
124+
// the (source, check) pair, so another source's silence never
125+
// applies; nor do canopy's own silences or other servers'.
124126
ServerSilencedRef::add(
125127
&mut conn,
126128
server_id,
@@ -130,31 +132,27 @@ async fn silenced_refs_with_prefix_combines_scopes_and_filters() {
130132
)
131133
.await
132134
.expect("other-source silence");
133-
// None of these may leak into the result: reserved-source silences
134-
// present at their bare refs (outside the health/ namespace), and
135-
// silences on other servers don't apply here at all.
136135
ServerSilencedRef::add(&mut conn, server_id, "canopy", "reachability", None)
137136
.await
138137
.expect("canopy silence");
139138
ServerSilencedRef::add(&mut conn, other_server_id, "alertd", "health/other", None)
140139
.await
141140
.expect("other-server silence");
142141

143-
let mut refs = silenced_refs_with_prefix(&mut conn, server_id, Some(group_id), "health/")
144-
.await
145-
.expect("refs");
146-
refs.sort();
142+
let checks =
143+
silenced_health_checks_for_server(&mut conn, server_id, Some(group_id), "alertd")
144+
.await
145+
.expect("checks");
147146
assert_eq!(
148-
refs,
149-
vec!["health/flaky", "health/groupwide", "health/other-source"]
147+
checks.into_iter().collect::<Vec<_>>(),
148+
vec!["flaky", "groupwide"]
150149
);
151150

152151
// Ungrouped lookup only sees the server-scope silences.
153-
let mut refs = silenced_refs_with_prefix(&mut conn, server_id, None, "health/")
152+
let checks = silenced_health_checks_for_server(&mut conn, server_id, None, "alertd")
154153
.await
155-
.expect("refs without group");
156-
refs.sort();
157-
assert_eq!(refs, vec!["health/flaky", "health/other-source"]);
154+
.expect("checks without group");
155+
assert_eq!(checks.into_iter().collect::<Vec<_>>(), vec!["flaky"]);
158156
})
159157
.await
160158
}

0 commit comments

Comments
 (0)