fix(core): refuse a squad replacement that would empty a populated squad
CI / Build, lint & test (push) Successful in 3m38s
CI / Build, lint & test (push) Successful in 3m38s
/squad/replace is a full replacement: it deletes every assignment and reinserts the supplied slots. Nothing validated that the supplied list was non-empty, so a caller sending no slots silently wiped the squad and got 200/ok back. This happened for real. A FIFA 17 client whose in-memory squad had been destroyed by a bad parse wrote its emptiness back twice; WAL forensics on the staging DB pin the damage to commit frame 465, squad_players 18 rows -> 0, logged as route=squad-replace status=200 outcome=ok. The squad is the authority's state, so mirroring a broken client's model is unrecoverable. No product flow empties a squad: a full-replacement client sends its complete slot array, and no caller or test in the tree builds an empty slot list. So an empty list means the caller's model is broken, and the write is refused with BadRequest. The check runs inside the transaction, so a concurrent write cannot slip between the count and the delete, and a newly created squad counts zero and is unaffected. The regression test asserts both halves: the empty replacement is rejected, and the existing assignments survive it. With the guard removed the test fails with 200 and slots_written 0 - the exact production symptom.
This commit is contained in:
@@ -345,6 +345,32 @@ async fn replace_squad_inner(
|
||||
}
|
||||
};
|
||||
|
||||
// A replacement carrying no slots would DELETE every assignment below and
|
||||
// insert nothing, silently emptying the squad. No product flow does that:
|
||||
// a full-replacement client sends its COMPLETE slot array, so an empty list
|
||||
// means the caller's own model was destroyed, not that the user emptied
|
||||
// their squad. Mirroring that damage into the authority is unrecoverable,
|
||||
// so refuse it.
|
||||
//
|
||||
// Observed for real: a FIFA 17 client whose in-memory squad had been
|
||||
// destroyed by a bad parse wrote its emptiness back twice, taking
|
||||
// `squad_players` from 18 rows to 0 while the request logged 200/ok.
|
||||
//
|
||||
// Checked inside the transaction so a concurrent write cannot slip between
|
||||
// the count and the delete. A newly created squad counts 0 and is unaffected.
|
||||
if replacement.slots.is_empty() {
|
||||
let existing =
|
||||
sqlx::query_scalar::<_, i64>("SELECT COUNT(*) FROM squad_players WHERE squad_id = ?")
|
||||
.bind(&squad_id)
|
||||
.fetch_one(&mut *tx)
|
||||
.await?;
|
||||
if existing > 0 {
|
||||
return Err(AppError::BadRequest(format!(
|
||||
"refusing to empty a populated squad: replacement carried no slots, but squad '{squad_id}' holds {existing} assignments"
|
||||
)));
|
||||
}
|
||||
}
|
||||
|
||||
sqlx::query("DELETE FROM squad_players WHERE squad_id = ?")
|
||||
.bind(&squad_id)
|
||||
.execute(&mut *tx)
|
||||
|
||||
Reference in New Issue
Block a user