From db8f8b02a038b7adcc1937c0914d6074587f1aa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Saparelli?= Date: Wed, 5 Aug 2026 09:59:49 +1200 Subject: [PATCH] fix(incidents): a membership row outliving its incident must not gate the next one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit is_issue_in_open_incident counted incident_issues rows by left_at IS NULL alone, never consulting the incident's closed_at. Closing an incident doesn't stamp left_at on the members that never left — a sub-failure contributor is held attached for context, and the close paths only touch the incidents row — so those rows outlive their incident and the count read them as live membership. An issue that already looks attached never opens an incident: its next effective failure arrives with was_in = true and should_leave = false, lands in the no-op arm, and the only thing that arm does is un-linger incidents where closed_at IS NULL, which excludes the closed one. Nothing opens, and no later event can clear the stale row, so the server stays red with nothing paging. In production this had stranded 207 issues across 51 servers, one of them failing unattended for seven weeks. The leave arm picked its membership row the same way, so it comes along: an issue can hold a stranded row and a live row at once, and stamping the stranded one weighs remaining_open against a long-closed incident and abandons the open one with no live members and no Slack resolve. Co-Authored-By: Claude Opus 5 (1M context) --- crates/database/src/issues.rs | 31 ++- .../tests/it/incident_stranded_membership.rs | 179 ++++++++++++++++++ crates/database/tests/it/main.rs | 1 + 3 files changed, 208 insertions(+), 3 deletions(-) create mode 100644 crates/database/tests/it/incident_stranded_membership.rs diff --git a/crates/database/src/issues.rs b/crates/database/src/issues.rs index 3f65d7e3..bbe09dee 100644 --- a/crates/database/src/issues.rs +++ b/crates/database/src/issues.rs @@ -1794,12 +1794,23 @@ async fn re_evaluate_incident_membership( } } (true, _, true) => { + // Must match `is_issue_in_open_incident`'s definition of live + // membership, incident `closed_at` included: an issue can hold an + // unstamped row on a *closed* incident (a close doesn't stamp the + // members that never left) at the same time as a live row on an + // open one. Filtering on `left_at` alone lets this pick the + // stranded row, and the leave then stamps the wrong membership and + // weighs `remaining_open` against a long-closed incident — + // abandoning the open one with no live members and no Slack + // resolve. let open_link: IncidentIssue = incident_issues::table + .inner_join(incidents::table.on(incidents::id.eq(incident_issues::incident_id))) .select(IncidentIssue::as_select()) .filter( incident_issues::issue_id .eq(issue.id) - .and(incident_issues::left_at.is_null()), + .and(incident_issues::left_at.is_null()) + .and(incidents::closed_at.is_null()), ) .for_update() .first(conn) @@ -2414,14 +2425,28 @@ pub async fn sweep_lingering_incidents(db: &mut AsyncPgConnection) -> Result Result { - use crate::schema::incident_issues; + use crate::schema::{incident_issues, incidents}; let count: i64 = incident_issues::table + .inner_join(incidents::table.on(incidents::id.eq(incident_issues::incident_id))) .filter( incident_issues::issue_id .eq(issue_id) - .and(incident_issues::left_at.is_null()), + .and(incident_issues::left_at.is_null()) + .and(incidents::closed_at.is_null()), ) .count() .get_result(db) diff --git a/crates/database/tests/it/incident_stranded_membership.rs b/crates/database/tests/it/incident_stranded_membership.rs new file mode 100644 index 00000000..26bda02a --- /dev/null +++ b/crates/database/tests/it/incident_stranded_membership.rs @@ -0,0 +1,179 @@ +//! Membership rows stranded in *closed* incidents must not gate future +//! incidents. +//! +//! Closing an incident retires it without stamping `left_at` on the members +//! that never left — a warning-level contributor is held attached for +//! context, and the close paths only ever touch the `incidents` row. Those +//! rows outlive their incident, so "is this issue in an open incident?" has +//! to consult the incident's `closed_at`, not just `left_at`. Reading +//! `left_at` alone makes a stranded issue look permanently attached, and an +//! issue that already appears attached never opens an incident when it +//! fails: the server goes red with nothing paging. + +use commons_types::status::CheckResult; +use database::issues::NewEvent; +use diesel::prelude::*; +use diesel::{QueryableByName, sql_query, sql_types}; +use diesel_async::RunQueryDsl; +use uuid::Uuid; + +#[derive(QueryableByName)] +struct RowId { + #[diesel(sql_type = sql_types::Uuid)] + id: Uuid, +} + +/// Server in a fresh group whose linger window is zero, so a recovery +/// closes the incident on the spot rather than leaving it to the sweep. +async fn insert_grouped_server(conn: &mut diesel_async::AsyncPgConnection) -> (Uuid, Uuid) { + let group: RowId = sql_query( + "INSERT INTO server_groups (name, slack_close_delay) \ + VALUES ('stranded-group', INTERVAL '0') RETURNING id", + ) + .get_result(conn) + .await + .expect("group"); + let server: RowId = sql_query( + "INSERT INTO servers (host, group_id) VALUES ('http://stranded.invalid/', $1) RETURNING id", + ) + .bind::(group.id) + .get_result(conn) + .await + .expect("server"); + (group.id, server.id) +} + +async fn save_event( + conn: &mut diesel_async::AsyncPgConnection, + server_id: Uuid, + r#ref: &str, + result: CheckResult, +) { + let active = matches!( + result, + CheckResult::Failed | CheckResult::Warning | CheckResult::Broken + ); + let stamp = database::issues::CheckStateStamp { + check: r#ref.into(), + observed: result, + effective: result, + escalates: false, + detail: None, + }; + NewEvent { + source: "test".into(), + r#ref: r#ref.into(), + description: None, + message: format!("{ref_} is {result:?}", ref_ = r#ref), + active: Some(active), + occurred_at: None, + } + .save_with_state(conn, server_id, None, Some(&stamp), false) + .await + .expect("save event"); +} + +async fn open_incident_count(conn: &mut diesel_async::AsyncPgConnection, group_id: Uuid) -> i64 { + use database::schema::incidents::dsl; + dsl::incidents + .filter(dsl::server_group_id.eq(group_id)) + .filter(dsl::closed_at.is_null()) + .count() + .get_result(conn) + .await + .expect("count open incidents") +} + +/// Count membership rows for `ref` that are still unstamped, regardless of +/// whether their incident is closed. +async fn unstamped_memberships( + conn: &mut diesel_async::AsyncPgConnection, + server_id: Uuid, + r#ref: &str, +) -> i64 { + #[derive(QueryableByName)] + struct Count { + #[diesel(sql_type = sql_types::BigInt)] + n: i64, + } + let row: Count = sql_query( + "SELECT count(*) AS n FROM incident_issues ii \ + JOIN issues i ON i.id = ii.issue_id \ + WHERE i.server_id = $1 AND i.ref = $2 AND ii.left_at IS NULL", + ) + .bind::(server_id) + .bind::(r#ref) + .get_result(conn) + .await + .expect("count memberships"); + row.n +} + +/// The regression: a warning contributor stranded by a close must still be +/// able to open a fresh incident when it later fails. +#[tokio::test(flavor = "multi_thread")] +async fn stranded_member_can_open_a_later_incident() { + commons_tests::db::TestDb::run(|mut conn, _url| async move { + let (group_id, server_id) = insert_grouped_server(&mut conn).await; + + // A failure opens the incident; a warning joins it as a lesser + // contributor because the target already has one open. + save_event( + &mut conn, + server_id, + "health/disk_free", + CheckResult::Failed, + ) + .await; + save_event( + &mut conn, + server_id, + "health/pg_tuning", + CheckResult::Warning, + ) + .await; + assert_eq!( + open_incident_count(&mut conn, group_id).await, + 1, + "the failure should have opened an incident", + ); + + // The failure recovers. With a zero linger window the incident + // closes immediately, and the warning contributor is left attached: + // this is the stranding, reproduced the way production makes it. + save_event( + &mut conn, + server_id, + "health/disk_free", + CheckResult::Passed, + ) + .await; + assert_eq!( + open_incident_count(&mut conn, group_id).await, + 0, + "the recovery should have closed the incident", + ); + assert_eq!( + unstamped_memberships(&mut conn, server_id, "health/pg_tuning").await, + 1, + "the warning contributor should still be attached to the closed incident", + ); + + // The stranded contributor now fails. Its stale membership names a + // closed incident, so it is not in an open one, and this failure has + // to open a new incident. + save_event( + &mut conn, + server_id, + "health/pg_tuning", + CheckResult::Failed, + ) + .await; + assert_eq!( + open_incident_count(&mut conn, group_id).await, + 1, + "a failure on a stranded issue must open a new incident", + ); + }) + .await; +} diff --git a/crates/database/tests/it/main.rs b/crates/database/tests/it/main.rs index b4aa5bcb..1fb48b48 100644 --- a/crates/database/tests/it/main.rs +++ b/crates/database/tests/it/main.rs @@ -26,6 +26,7 @@ mod incident_open_race; mod incident_reeval_queue; mod incident_result_semantics; mod incident_stats; +mod incident_stranded_membership; mod issue_list_filters; mod mcp_tokens; mod migration_test_candidates;