diff --git a/crates/database/src/issues.rs b/crates/database/src/issues.rs index bbe09dee..7b07fd3b 100644 --- a/crates/database/src/issues.rs +++ b/crates/database/src/issues.rs @@ -1892,6 +1892,7 @@ async fn re_evaluate_incident_membership( .await .optional()?; if let Some(closed) = closed { + release_remaining_members(conn, closed.id, transition_time).await?; enqueue_slack_resolve_inner(conn, &closed, by).await?; } } else { @@ -2414,6 +2415,9 @@ pub async fn sweep_lingering_incidents(db: &mut AsyncPgConnection) -> Result Result Result<()> { + use crate::schema::incident_issues; + + diesel::update( + incident_issues::table + .filter(incident_issues::incident_id.eq(incident_id)) + .filter(incident_issues::left_at.is_null()), + ) + .set(incident_issues::left_at.eq(jiff_diesel::Timestamp::from(at))) + .execute(conn) + .await?; + Ok(()) +} + /// Is this issue currently a live member of an incident that is still open? /// /// Both halves matter. `left_at IS NULL` alone is not membership in an *open* @@ -3586,6 +3622,10 @@ impl Incident { .returning(Incident::as_select()) .get_result(conn) .await?; + // The per-issue resolves above drive most members out through the + // leave arm, but one already carrying `resolved_at` is skipped to + // keep its audit trail, and never leaves. Release whatever is left. + release_remaining_members(conn, incident.id, now).await?; enqueue_slack_resolve(conn, &incident, by).await?; Ok(incident) }) diff --git a/crates/database/tests/it/incident_stranded_membership.rs b/crates/database/tests/it/incident_stranded_membership.rs index 26bda02a..c2e92149 100644 --- a/crates/database/tests/it/incident_stranded_membership.rs +++ b/crates/database/tests/it/incident_stranded_membership.rs @@ -1,30 +1,45 @@ -//! Membership rows stranded in *closed* incidents must not gate future -//! incidents. +//! Membership rows must not outlive the incident they name. //! -//! 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. +//! An incident close only retires the `incidents` row. The issue whose +//! recovery triggered the close is stamped by the leave arm, but sub-failure +//! contributors are held attached for context, and nothing released them — +//! their `incident_issues` rows survived the incident claiming a membership +//! that had ended. +//! +//! That stranded a check outside the incident workflow for good. Live +//! membership is what decides whether a failure opens an incident, and an +//! issue that already looks attached never opens one, so the server sat red +//! with nothing paging and no later event could clear the stale row. +//! +//! Covered here from both sides: the close paths now release whoever is left, +//! and the membership read consults the incident's `closed_at` so any row +//! that predates the fix is inert. use commons_types::status::CheckResult; use database::issues::NewEvent; use diesel::prelude::*; use diesel::{QueryableByName, sql_query, sql_types}; -use diesel_async::RunQueryDsl; +use diesel_async::{RunQueryDsl, SimpleAsyncConnection as _}; use uuid::Uuid; +const BACKFILL_UP: &str = include_str!( + "../../../../migrations/2026-08-04-220209-0000_release_stranded_incident_members/up.sql" +); + #[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. +#[derive(QueryableByName)] +struct Count { + #[diesel(sql_type = sql_types::BigInt)] + n: i64, +} + +/// 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) \ @@ -84,18 +99,27 @@ async fn open_incident_count(conn: &mut diesel_async::AsyncPgConnection, group_i .expect("count open incidents") } -/// Count membership rows for `ref` that are still unstamped, regardless of -/// whether their incident is closed. +async fn issue_id( + conn: &mut diesel_async::AsyncPgConnection, + server_id: Uuid, + r#ref: &str, +) -> Uuid { + let row: RowId = sql_query("SELECT id FROM issues WHERE server_id = $1 AND ref = $2") + .bind::(server_id) + .bind::(r#ref) + .get_result(conn) + .await + .expect("issue id"); + row.id +} + +/// 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 \ @@ -109,10 +133,30 @@ async fn unstamped_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. +/// Re-open every membership row for `ref`, reproducing the rows the close +/// paths used to leave behind. +async fn stranded_as_legacy_data( + conn: &mut diesel_async::AsyncPgConnection, + server_id: Uuid, + r#ref: &str, +) { + sql_query( + "UPDATE incident_issues ii SET left_at = NULL \ + FROM issues i WHERE i.id = ii.issue_id \ + AND i.server_id = $1 AND i.ref = $2", + ) + .bind::(server_id) + .bind::(r#ref) + .execute(conn) + .await + .expect("strand membership"); +} + +/// The close side: a contributor that never left of its own accord is +/// released when the incident closes, so its membership ends with the +/// incident rather than outliving it. #[tokio::test(flavor = "multi_thread")] -async fn stranded_member_can_open_a_later_incident() { +async fn closing_an_incident_releases_the_members_that_never_left() { commons_tests::db::TestDb::run(|mut conn, _url| async move { let (group_id, server_id) = insert_grouped_server(&mut conn).await; @@ -137,10 +181,57 @@ async fn stranded_member_can_open_a_later_incident() { 1, "the failure should have opened an incident", ); + assert_eq!( + unstamped_memberships(&mut conn, server_id, "health/pg_tuning").await, + 1, + "the warning should be a live member while the incident is open", + ); + + // The failure recovers and, with a zero linger window, closes the + // incident on the spot. The warning never left by itself. + 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, + 0, + "the close should have released the warning contributor", + ); + }) + .await; +} + +/// The read side: a row left over from before the close paths released their +/// members names a closed incident, so it is not live membership and must not +/// stop a later failure from opening an incident. +#[tokio::test(flavor = "multi_thread")] +async fn a_stranded_member_can_still_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; - // 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::Failed, + ) + .await; + save_event( + &mut conn, + server_id, + "health/pg_tuning", + CheckResult::Warning, + ) + .await; save_event( &mut conn, server_id, @@ -153,15 +244,16 @@ async fn stranded_member_can_open_a_later_incident() { 0, "the recovery should have closed the incident", ); + + // Put the warning's membership back the way the old close paths left + // it: attached to an incident that closed weeks ago. + stranded_as_legacy_data(&mut conn, server_id, "health/pg_tuning").await; 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 row is the state under test", ); - // 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, @@ -177,3 +269,69 @@ async fn stranded_member_can_open_a_later_incident() { }) .await; } + +/// The backfill retires rows whose incident has closed and leaves live +/// membership alone. +#[tokio::test(flavor = "multi_thread")] +async fn the_backfill_releases_stranded_members_and_spares_live_ones() { + commons_tests::db::TestDb::run(|mut conn, _url| async move { + let (group_id, server_id) = insert_grouped_server(&mut conn).await; + + // Two warnings, so neither opens an incident on its own and both are + // free to be linked by hand below. + save_event(&mut conn, server_id, "health/stale", CheckResult::Warning).await; + save_event(&mut conn, server_id, "health/live", CheckResult::Warning).await; + let stale_issue = issue_id(&mut conn, server_id, "health/stale").await; + let live_issue = issue_id(&mut conn, server_id, "health/live").await; + + let closed: RowId = sql_query( + "INSERT INTO incidents (server_group_id, opened_at, closed_at) \ + VALUES ($1, now() - INTERVAL '3 days', now() - INTERVAL '2 days') RETURNING id", + ) + .bind::(group_id) + .get_result(&mut conn) + .await + .expect("closed incident"); + let open: RowId = sql_query( + "INSERT INTO incidents (server_group_id, opened_at) \ + VALUES ($1, now() - INTERVAL '1 hour') RETURNING id", + ) + .bind::(group_id) + .get_result(&mut conn) + .await + .expect("open incident"); + + for (incident, issue) in [(closed.id, stale_issue), (open.id, live_issue)] { + sql_query( + "INSERT INTO incident_issues (incident_id, issue_id, joined_at) \ + VALUES ($1, $2, now() - INTERVAL '3 days')", + ) + .bind::(incident) + .bind::(issue) + .execute(&mut conn) + .await + .expect("link issue"); + } + + conn.batch_execute(BACKFILL_UP).await.expect("backfill"); + + let stale_matches_close: Count = sql_query( + "SELECT count(*) AS n FROM incident_issues ii JOIN incidents i ON i.id = ii.incident_id \ + WHERE ii.issue_id = $1 AND ii.left_at = i.closed_at", + ) + .bind::(stale_issue) + .get_result(&mut conn) + .await + .expect("stale row"); + assert_eq!( + stale_matches_close.n, 1, + "the stranded row should have left when its incident closed", + ); + assert_eq!( + unstamped_memberships(&mut conn, server_id, "health/live").await, + 1, + "membership of a still-open incident is live and must be untouched", + ); + }) + .await; +} diff --git a/migrations/2026-08-04-220209-0000_release_stranded_incident_members/down.sql b/migrations/2026-08-04-220209-0000_release_stranded_incident_members/down.sql new file mode 100644 index 00000000..999eab3c --- /dev/null +++ b/migrations/2026-08-04-220209-0000_release_stranded_incident_members/down.sql @@ -0,0 +1,8 @@ +-- Deliberately a no-op. +-- +-- Nothing distinguishes a row this migration stamped from one the close paths +-- stamped afterwards, so any revert would clear legitimate leaves along with +-- the backfill and re-strand the memberships. Leaving `left_at` set is also +-- harmless to the older code, which only ever asked whether it was NULL. + +SELECT 1; diff --git a/migrations/2026-08-04-220209-0000_release_stranded_incident_members/up.sql b/migrations/2026-08-04-220209-0000_release_stranded_incident_members/up.sql new file mode 100644 index 00000000..89b189c6 --- /dev/null +++ b/migrations/2026-08-04-220209-0000_release_stranded_incident_members/up.sql @@ -0,0 +1,17 @@ +-- Release membership rows that outlived the incident they name. +-- +-- Closing an incident never stamped `left_at` on the members that hadn't left +-- of their own accord, so a sub-failure contributor stayed attached to an +-- incident that had been closed for weeks. Those rows read as live +-- membership, which kept the issue from ever opening another incident: a +-- server could sit red with nothing paging. +-- +-- The membership ended when the incident closed, so that is what `left_at` +-- becomes. Rows whose incident is still open are live and are left alone. + +UPDATE incident_issues ii +SET left_at = i.closed_at +FROM incidents i +WHERE i.id = ii.incident_id + AND ii.left_at IS NULL + AND i.closed_at IS NOT NULL;