fix(incidents): a close releases the members that never left

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<usi
 						.returning(Incident::as_select())
 						.get_result(conn)
 						.await?;
+				// Backdated to the close, matching `closed_at`: the linger is
+				// damping machinery, not part of the incident's span.
+				release_remaining_members(conn, closed_incident.id, closing_at).await?;
 				enqueue_slack_resolve_inner(conn, &closed_incident, None).await?;
 				Ok(true)
 			})
@@ -2425,6 +2429,38 @@ pub async fn sweep_lingering_incidents(db: &mut AsyncPgConnection) -> Result<usi
 	Ok(closed)
 }
 
+/// Stamp `left_at` on every issue still attached to `incident_id`, so no
+/// membership row outlives the incident it names.
+///
+/// A close only retires the `incidents` row: the current issue's own leave is
+/// stamped by the leave arm, but sub-failure contributors are held attached
+/// for context and nothing ever released them. Their rows then claimed a
+/// membership that had ended, which is both untrue — the membership ran from
+/// `joined_at` to the close, and `left_at` is where that end is recorded — and
+/// the thing that used to strand an issue permanently outside the incident
+/// workflow.
+///
+/// Call after the close, with the same timestamp the close was recorded at, so
+/// the membership ends exactly when the incident did. Idempotent: a row that
+/// already left keeps its own earlier stamp.
+async fn release_remaining_members(
+	conn: &mut AsyncPgConnection,
+	incident_id: Uuid,
+	at: Timestamp,
+) -> 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)
 		})

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::<sql_types::Uuid, _>(server_id)
+		.bind::<sql_types::Text, _>(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::<sql_types::Uuid, _>(server_id)
+	.bind::<sql_types::Text, _>(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::<sql_types::Uuid, _>(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::<sql_types::Uuid, _>(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::<sql_types::Uuid, _>(incident)
+			.bind::<sql_types::Uuid, _>(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::<sql_types::Uuid, _>(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;
+}

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;

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;