fix(me): two hosts deleting at once can no longer leave the event with no operator
The last-operator guard ran on the pool, before the transaction opened. Two hosts deleting themselves at the same moment each saw the other, both passed, and the event was left with nobody who can moderate, nobody who can release the gallery, and no way to appoint anyone — because appointing requires a host. Not recoverable from inside the app. The fix is a transaction-scoped ADVISORY lock, and the two obvious alternatives are both worse: * `FOR UPDATE` on the other operators' rows DEADLOCKS. Each deleter locks the other's row and then tries to delete its own, so Postgres resolves it by killing one. The invariant survives; the loser gets a 500 instead of the sentence explaining what to do. My first attempt did exactly this, and the test caught it. * Locking the `event` row serialises cleanly but inverts the lock order every moderation path takes (upload/user rows first, event last). That is an ABBA against a path that runs constantly during the event, traded for one that runs approximately never. An advisory lock is a separate lock space, so it cannot interact with the row-lock graph at all, and it is released when the transaction ends. The loser waits, counts zero once the winner's row is gone, and is refused with the sentence it should have got. The test is a genuine concurrency test — it spawns the second deleter and asserts it unblocks to see no remaining operator. It fails against the pre-check-outside-the-transaction version and against the FOR UPDATE version.
This commit is contained in:
@@ -199,6 +199,48 @@ pub async fn delete_account(
|
|||||||
crate::services::export::Affects::Both,
|
crate::services::export::Affects::Both,
|
||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
|
// The last-host guard again, now AUTHORITATIVELY — inside the transaction, holding a lock.
|
||||||
|
//
|
||||||
|
// The check above runs on the pool before this transaction opens, so two hosts deleting
|
||||||
|
// themselves at the same moment each saw the other and both proceeded, leaving the event with
|
||||||
|
// NO operator: nobody to moderate, nobody to release the gallery, and no way to appoint anyone
|
||||||
|
// because appointing requires a host. That is not recoverable from inside the app.
|
||||||
|
//
|
||||||
|
// Serialised with a transaction-scoped ADVISORY lock, not a row lock.
|
||||||
|
//
|
||||||
|
// `FOR UPDATE` on the other operators' rows looks like the obvious answer and is the wrong one:
|
||||||
|
// each deleter would lock the OTHER's row and then try to delete its own, so the two block on
|
||||||
|
// each other and Postgres resolves it by killing one with a deadlock error — the invariant
|
||||||
|
// holds, but the loser gets a 500 instead of the sentence below. Locking the `event` row
|
||||||
|
// instead would serialise cleanly, but it inverts the lock order every moderation path uses
|
||||||
|
// (upload/user rows first, event last), which is an ABBA waiting to happen.
|
||||||
|
//
|
||||||
|
// An advisory lock has neither problem: it is a separate lock space, so it cannot interact with
|
||||||
|
// the row-lock graph at all, and it is released automatically when this transaction ends.
|
||||||
|
// 4242 is an arbitrary namespace to keep this key from colliding with any future advisory use.
|
||||||
|
if matches!(user.role, UserRole::Host | UserRole::Admin) {
|
||||||
|
sqlx::query("SELECT pg_advisory_xact_lock(4242, hashtext($1::text))")
|
||||||
|
.bind(auth.event_id)
|
||||||
|
.execute(&mut *tx)
|
||||||
|
.await?;
|
||||||
|
let others: Vec<uuid::Uuid> = sqlx::query_scalar(
|
||||||
|
"SELECT id FROM \"user\"
|
||||||
|
WHERE event_id = $1 AND id != $2
|
||||||
|
AND role IN ('host', 'admin') AND is_banned = FALSE",
|
||||||
|
)
|
||||||
|
.bind(auth.event_id)
|
||||||
|
.bind(auth.user_id)
|
||||||
|
.fetch_all(&mut *tx)
|
||||||
|
.await?;
|
||||||
|
if others.is_empty() {
|
||||||
|
return Err(AppError::BadRequest(
|
||||||
|
"Du bist der letzte Gastgeber. Ernenne zuerst einen anderen Gastgeber, bevor du \
|
||||||
|
dein Konto löschst."
|
||||||
|
.into(),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// And the account. `session`, `like` and `pin_reset_request` cascade from here.
|
// And the account. `session`, `like` and `pin_reset_request` cascade from here.
|
||||||
sqlx::query("DELETE FROM \"user\" WHERE id = $1")
|
sqlx::query("DELETE FROM \"user\" WHERE id = $1")
|
||||||
.bind(auth.user_id)
|
.bind(auth.user_id)
|
||||||
|
|||||||
@@ -295,3 +295,99 @@ async fn upload_after_release_commits_sees_the_lock_and_is_rejected(pool: PgPool
|
|||||||
);
|
);
|
||||||
tx.rollback().await.unwrap();
|
tx.rollback().await.unwrap();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// SRC: `handlers/me.rs::delete_account` — the last-operator guard, verbatim.
|
||||||
|
///
|
||||||
|
/// Returns the ids of the OTHER live operators, holding a row lock on each. The handler refuses the
|
||||||
|
/// deletion when this is empty.
|
||||||
|
async fn other_operators(tx: &mut sqlx::PgConnection, event_id: Uuid, self_id: Uuid) -> Vec<Uuid> {
|
||||||
|
sqlx::query("SELECT pg_advisory_xact_lock(4242, hashtext($1::text))")
|
||||||
|
.bind(event_id)
|
||||||
|
.execute(&mut *tx)
|
||||||
|
.await
|
||||||
|
.expect("advisory lock");
|
||||||
|
sqlx::query_scalar(
|
||||||
|
"SELECT id FROM \"user\"
|
||||||
|
WHERE event_id = $1 AND id != $2
|
||||||
|
AND role IN ('host', 'admin') AND is_banned = FALSE",
|
||||||
|
)
|
||||||
|
.bind(event_id)
|
||||||
|
.bind(self_id)
|
||||||
|
.fetch_all(tx)
|
||||||
|
.await
|
||||||
|
.expect("other_operators")
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Two hosts deleting themselves at the same moment must not both succeed.
|
||||||
|
///
|
||||||
|
/// The guard used to run on the pool BEFORE the transaction opened, so each deleter saw the other,
|
||||||
|
/// both passed, and the event was left with no operator at all — nobody to moderate, nobody to
|
||||||
|
/// release the gallery, and no way to appoint anyone, because appointing requires a host. Not
|
||||||
|
/// recoverable from inside the app.
|
||||||
|
///
|
||||||
|
/// A transaction-scoped ADVISORY lock serialises them. A row lock on the other operators would
|
||||||
|
/// deadlock instead — each deleter locks the other's row and then tries to delete its own, so
|
||||||
|
/// Postgres kills one with a deadlock error; the invariant survives but the loser gets a 500. A
|
||||||
|
/// lock on the `event` row would serialise cleanly but inverts the order every moderation path
|
||||||
|
/// takes (upload/user rows first, event last). The advisory lock is a separate space, so it cannot
|
||||||
|
/// interact with the row-lock graph at all: the loser waits, then counts zero once the winner's row
|
||||||
|
/// is gone, and is refused with a sentence instead of an error.
|
||||||
|
#[sqlx::test]
|
||||||
|
async fn two_hosts_deleting_at_once_cannot_both_leave_the_event(pool: PgPool) {
|
||||||
|
let event_id = seed_event(&pool, "wedding").await;
|
||||||
|
let a = seed_user(&pool, event_id, "Gastgeber Anton").await;
|
||||||
|
let b = seed_user(&pool, event_id, "Gastgeberin Berta").await;
|
||||||
|
for id in [a, b] {
|
||||||
|
sqlx::query("UPDATE \"user\" SET role = 'host' WHERE id = $1")
|
||||||
|
.bind(id)
|
||||||
|
.execute(&pool)
|
||||||
|
.await
|
||||||
|
.expect("promote");
|
||||||
|
}
|
||||||
|
|
||||||
|
// A opens first and takes the lock on B's row.
|
||||||
|
let mut tx_a = pool.begin().await.expect("tx a");
|
||||||
|
let a_sees = other_operators(&mut tx_a, event_id, a).await;
|
||||||
|
assert_eq!(a_sees, vec![b], "A must see B as the remaining operator");
|
||||||
|
|
||||||
|
// B now tries the same and blocks on A's row. Spawned, because it cannot return until A
|
||||||
|
// commits — which is precisely the serialisation under test.
|
||||||
|
let pool_b = pool.clone();
|
||||||
|
let b_task = tokio::spawn(async move {
|
||||||
|
let mut tx_b = pool_b.begin().await.expect("tx b");
|
||||||
|
let seen = other_operators(&mut tx_b, event_id, b).await;
|
||||||
|
tx_b.commit().await.expect("commit b");
|
||||||
|
seen
|
||||||
|
});
|
||||||
|
|
||||||
|
// Give B a moment to actually reach the lock rather than racing past it.
|
||||||
|
tokio::time::sleep(Duration::from_millis(300)).await;
|
||||||
|
|
||||||
|
// A completes its deletion.
|
||||||
|
sqlx::query("DELETE FROM \"user\" WHERE id = $1")
|
||||||
|
.bind(a)
|
||||||
|
.execute(&mut *tx_a)
|
||||||
|
.await
|
||||||
|
.expect("delete a");
|
||||||
|
tx_a.commit().await.expect("commit a");
|
||||||
|
|
||||||
|
let b_sees = b_task.await.expect("b task");
|
||||||
|
assert!(
|
||||||
|
b_sees.is_empty(),
|
||||||
|
"B unblocked and must now see NO remaining operator (A is gone), so its deletion is \
|
||||||
|
refused — it saw {b_sees:?}"
|
||||||
|
);
|
||||||
|
|
||||||
|
// The event still has exactly one operator: B.
|
||||||
|
let remaining: i64 = sqlx::query_scalar(
|
||||||
|
"SELECT COUNT(*) FROM \"user\" WHERE event_id = $1 AND role IN ('host','admin')",
|
||||||
|
)
|
||||||
|
.bind(event_id)
|
||||||
|
.fetch_one(&pool)
|
||||||
|
.await
|
||||||
|
.expect("count");
|
||||||
|
assert_eq!(
|
||||||
|
remaining, 1,
|
||||||
|
"the event must never be left without an operator"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user