fabi/EventSnap
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(backend): three ways the end of the night could go wrong

Browse Source 
**1. Releasing the gallery could arm the keepsake with no worker.**
`release_gallery` ran `tx.commit()` -> SSE `event-closed` -> `audit::record().await`
-> `spawn_export_jobs`. The audit write is two pool round-trips, each able to wait
the full 5s acquire timeout, and it runs in the same instant `event-closed` fans
out to ~100 phones whose upload queues all hit the API at once. Axum drops the
handler future when the client disconnects — the host taps "Freigeben" and
pockets the phone. The release has COMMITTED: event closed, uploads locked, epoch
bumped, both `export_job` rows pending, and no worker. `/export/*` 404s, the page
sits on "Wird vorbereitet…", `recover_exports` only runs at boot, and a second
release is refused. Every other regen call site spawns first; `me.rs` says so in
a comment. This was the sole violator, and the only path that arms the FIRST
build of the keepsake. Spawn moved immediately after the commit.

**2. The event could be left with no operator.**
`remaining_operators` was an unlocked pool COUNT followed by a separate UPDATE,
so `ban_user` and `set_role` raced each other and `DELETE /me`: an admin demotes
host B while host A deletes themselves, each check sees the other still present,
both commit, and nobody can moderate, release the gallery, or appoint anyone —
appointing requires being an operator. The count now runs inside the writing
transaction behind the same advisory lock `delete_account` uses, via one shared
helper so the key cannot drift between copies.

The lock is taken FIRST in all three, and the order is load-bearing:
`delete_account` previously took it last, after row locks on `upload` and
`event`, while the two new call sites take it before locking those same rows —
an ABBA that Postgres would resolve by killing one transaction with a 500. The
ordering rule is documented on the helper.

**3. The keepsake could become unbuildable the moment uploads stopped.**
The upload gate and the export preflight computed the IDENTICAL threshold
(`required_free_bytes(media, 2) + DISK_RESERVE_BYTES`), leaving zero margin
between them. Once the gate refused its first upload the preflight was already at
its own limit, so anything written afterwards decided the keepsake's fate: WAL up
to `max_wal_size`, 30 MB x 4 of container logs, and the compression backlog
draining at exactly that hour. The release commits before the workers bail, so
the failure lands at 01:00 with no second release possible. The gate now demands
`UPLOAD_GATE_HEADROOM_BYTES` more than the preflight, costing ~0.5 GB of media
ceiling — the trade README already argues for. The dashboard banner mirrors the
new threshold so its lead is unchanged, and a new test pins gate-before-preflight
at six gallery sizes.

Also: the global disk gate fails OPEN when the mount cannot be read, which is
deliberate, but did it SILENTLY — no log line at all, while the export preflight
warns on the identical condition. Inside a container `/` is an overlay rather
than a `/dev` device, so this is reachable, and when it happens the only global
disk bound is gone and the box fills until Postgres cannot write WAL.

README's sizing table was also arithmetically self-contradictory (it showed
~27 GB free against a ~27.6 GB requirement); recomputed for the new gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
fabi
2026-08-13 19:45:24 +02:00
parent 06e0bea0e9
commit 20c15c3500
4 changed files with 265 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files

185
backend/src/handlers/host.rs
View File

@@ -57,7 +57,8 @@ pub struct EventStatus {
/// into a decision someone can still make.
///
/// IT MUST FIRE BEFORE THE UPLOAD GATE CLOSES, and that is why the reserve and the margin are
/// here. The gate in `handlers::upload` refuses at `free < keepsake_required + DISK_RESERVE_BYTES`;
/// here. The gate in `handlers::upload` refuses at
/// `free < keepsake_required + DISK_RESERVE_BYTES + UPLOAD_GATE_HEADROOM_BYTES`;
/// warning at `free < keepsake_required` alone meant the two differed by the whole reserve, so
/// the wall was always hit FIRST. Every guest would be blocked from uploading while this
/// dashboard showed a comfortable disk and no banner at all — on the shipped 40 GB box, uploads
@@ -66,8 +67,14 @@ pub struct EventStatus {
/// The 25% margin makes it a warning rather than an obituary: the host sees it while there is
/// still room to act (delete a few large videos, which refunds immediately and reopens the gate).
fn disk_is_low(free: u64, keepsake_required: u64) -> bool {
let gate_closes_at =
keepsake_required.saturating_add(crate::handlers::upload::DISK_RESERVE_BYTES as u64);
// Mirrors the gate EXACTLY, headroom included. The gate now demands
// `UPLOAD_GATE_HEADROOM_BYTES` more than the export preflight does, so that ordinary
// end-of-night writes cannot flip the preflight after uploads have already stopped. Leaving
// that term out here would shrink the warning's lead by 1.5 GB — and the whole point of this
// function is that the banner must appear while the host can still act.
let gate_closes_at = keepsake_required
.saturating_add(crate::handlers::upload::DISK_RESERVE_BYTES as u64)
.saturating_add(crate::handlers::upload::UPLOAD_GATE_HEADROOM_BYTES as u64);
let warn_at = gate_closes_at.saturating_add(gate_closes_at / 4);
// No separate absolute-floor clause. There used to be `free < LOW_DISK_FLOOR_BYTES ||` here,
// and it was unreachable: `gate_closes_at` is at least DISK_RESERVE_BYTES, so `warn_at` is at
@@ -80,8 +87,15 @@ fn disk_is_low(free: u64, keepsake_required: u64) -> bool {
/// Count non-banned hosts/admins in the event OTHER than `excluding` — the operators
/// who would remain if `excluding` were demoted or banned. Used to enforce the "an event
/// always keeps at least one operator" floor.
///
/// Takes a CONNECTION, not the pool, and every caller passes the same transaction it is about to
/// write in — after taking [`lock_operator_floor`]. Read on the pool beforehand, this count was a
/// snapshot that any concurrent operator-removing action could invalidate before the UPDATE landed:
/// an admin demoting host B while host A calls `DELETE /me` saw two independent checks each observe
/// the other still present, both commit, and the event end up with zero operators — which is not
/// recoverable from inside the app, since appointing an operator requires being one.
async fn remaining_operators(
state: &AppState,
conn: &mut sqlx::PgConnection,
event_id: Uuid,
excluding: Uuid,
) -> Result<i64, AppError> {
@@ -92,11 +106,36 @@ async fn remaining_operators(
)
.bind(event_id)
.bind(excluding)
.fetch_one(&state.pool)
.fetch_one(conn)
.await?;
Ok(count)
}
/// Serialise every action that can remove an operator from an event.
///
/// The same key `me::delete_account` takes — namespace 4242, `hashtext(event_id)` — and it MUST
/// stay identical, or the two families of caller lock against nothing. An advisory lock is used
/// rather than a row lock because it is a separate lock space and so cannot join the
/// `event`/`user` row-lock graph that moderation traffic already traverses in both directions;
/// it is released automatically when the transaction ends.
///
/// **Call this FIRST in the transaction, before taking any row lock.** Being a separate lock space
/// means it cannot form a cycle *with itself*, not that ordering is free: all three callers go on
/// to lock `user` and `event` rows, so a caller that took those rows first and reached for this
/// lock afterwards would deadlock against one that did it the other way round. Postgres would
/// break the tie by killing one transaction with a 500. Every caller acquires it first; keep it
/// that way.
pub(crate) async fn lock_operator_floor(
conn: &mut sqlx::PgConnection,
event_id: Uuid,
) -> Result<(), AppError> {
sqlx::query("SELECT pg_advisory_xact_lock(4242, hashtext($1::text))")
.bind(event_id)
.execute(conn)
.await?;
Ok(())
}
#[derive(Deserialize)]
pub struct SetRoleRequest {
pub role: String,
@@ -193,14 +232,6 @@ pub async fn ban_user(
));
}
// Floor: never leave the event with zero operators. Banning removes the target from
// the active-operator pool, so refuse if they're the last non-banned host/admin.
if target.0 == "host" && remaining_operators(&state, auth.event_id, user_id).await? == 0 {
return Err(AppError::BadRequest(
"Der letzte Host kann nicht gesperrt werden.".into(),
));
}
// Ban ALWAYS hides: a banned user's content is "gone" everywhere. The visibility
// views/queries now also filter on `is_banned` (defense in depth), and we set
// `uploads_hidden` so the existing `user-hidden` live-eviction path fires too. The old
@@ -215,6 +246,21 @@ pub async fn ban_user(
//
// The ban and the keepsake invalidation are ONE transaction — see `host_delete_upload`.
let mut tx = state.pool.begin().await?;
// Floor: never leave the event with zero operators. Banning removes the target from the
// active-operator pool, so refuse if they're the last non-banned host/admin.
//
// INSIDE the transaction and behind the operator lock — see `remaining_operators`. Checked on
// the pool beforehand, this raced `set_role` and `DELETE /me` into an event with no operator.
if target.0 == "host" {
lock_operator_floor(&mut tx, auth.event_id).await?;
if remaining_operators(&mut tx, auth.event_id, user_id).await? == 0 {
return Err(AppError::BadRequest(
"Der letzte Host kann nicht gesperrt werden.".into(),
));
}
}
sqlx::query(
"UPDATE \"user\"
SET is_banned = TRUE, uploads_hidden = TRUE, uploads_hidden_at = NOW()
@@ -461,21 +507,26 @@ pub async fn set_role(
// Floor: demoting the last non-banned host/admin to guest would leave the event with
// no operator. Refuse.
if new_role == "guest"
&& target.0 == "host"
&& remaining_operators(&state, auth.event_id, user_id).await? == 0
{
return Err(AppError::BadRequest(
"Der letzte Host kann nicht zum Gast gemacht werden.".into(),
));
//
// The check and the UPDATE are ONE transaction, behind the operator lock — see
// `remaining_operators`. Split apart on the pool, this raced `ban_user` and `DELETE /me`.
let mut tx = state.pool.begin().await?;
if new_role == "guest" && target.0 == "host" {
lock_operator_floor(&mut tx, auth.event_id).await?;
if remaining_operators(&mut tx, auth.event_id, user_id).await? == 0 {
return Err(AppError::BadRequest(
"Der letzte Host kann nicht zum Gast gemacht werden.".into(),
));
}
}
sqlx::query("UPDATE \"user\" SET role = $2::user_role WHERE id = $1 AND event_id = $3")
.bind(user_id)
.bind(new_role)
.bind(auth.event_id)
.execute(&state.pool)
.execute(&mut *tx)
.await?;
tx.commit().await?;
tracing::info!(
actor_user_id = %auth.user_id,
target_user_id = %user_id,
@@ -938,9 +989,39 @@ pub async fn release_gallery(
// discovering it via a rejected upload.
let _ = state.sse_tx.send(SseEvent::new("event-closed", "{}"));
// Detached — survives this handler being cancelled.
//
// SPAWNED IMMEDIATELY AFTER THE COMMIT, BEFORE ANY OTHER `.await`. Every `invalidate_and_arm`
// call site does this; `me::delete_account` carries the same note. The audit write below used
// to sit here, and it is two pool round-trips that can each wait up to the 5 s acquire timeout
// — right at the moment `event-closed` has just fanned out to ~100 phones whose queues all hit
// the API at once, so the pool is as contended as it ever gets. Drop the handler future during
// that suspension (the host's phone sleeps, the tab closes, Caddy times the request out) and
// the task never spawns: the event is released, uploads are locked, both `export_job` rows sit
// `pending` at the live epoch, and no worker exists. `/export/*` 404s, the page sits on "Wird
// vorbereitet…", `recover_exports` only runs at boot, and `release_gallery` refuses a retry
// because the gallery is already released.
//
// This is the one path that arms the FIRST build of the keepsake, so it is the worst possible
// place to reintroduce that window.
crate::services::export::spawn_export_jobs(
event_id,
event_name,
epoch,
state.config.comments_enabled,
std::time::Duration::ZERO,
state.pool.clone(),
state.config.media_path.clone(),
state.config.export_path.clone(),
state.sse_tx.clone(),
);
// Was logged NOWHERE at all before this — not even a tracing line. A host reading
// the record the morning after had no way to see when uploads were locked or the
// gallery released, which are the two actions that change what every guest can do.
//
// Last, deliberately: it is best-effort by design (it swallows its own errors), so nothing
// downstream may depend on it having completed.
crate::services::audit::record(
&state.pool,
auth.event_id,
@@ -954,26 +1035,13 @@ pub async fn release_gallery(
)
.await;
// Detached — survives this handler being cancelled.
crate::services::export::spawn_export_jobs(
event_id,
event_name,
epoch,
state.config.comments_enabled,
std::time::Duration::ZERO,
state.pool.clone(),
state.config.media_path.clone(),
state.config.export_path.clone(),
state.sse_tx.clone(),
);
Ok(StatusCode::NO_CONTENT)
}
#[cfg(test)]
mod tests {
use super::disk_is_low;
use crate::handlers::upload::DISK_RESERVE_BYTES;
use crate::handlers::upload::{DISK_RESERVE_BYTES, UPLOAD_GATE_HEADROOM_BYTES};
use crate::services::export::required_free_bytes;
const GB: u64 = 1_000_000_000;
@@ -1016,7 +1084,8 @@ mod tests {
// require it to be strictly above the level at which the gate closes, by a usable amount.
for media_gb in [0u64, 1, 4, 8, 16, 32] {
let required = required_free_bytes(media_gb * GB, 2);
let gate_closes_at = required + DISK_RESERVE_BYTES as u64;
let gate_closes_at =
required + DISK_RESERVE_BYTES as u64 + UPLOAD_GATE_HEADROOM_BYTES as u64;
// Just above the gate: guests can still upload, and the host must already be warned.
assert!(
@@ -1037,6 +1106,43 @@ mod tests {
}
}
/// The invariant the headroom exists for: uploads must stop while the keepsake can STILL be
/// built, with room to spare — not at the exact instant the preflight reaches its own limit.
///
/// Both thresholds used to be `required_free_bytes(media, 2) + DISK_RESERVE_BYTES`, identically.
/// So the moment the gate refused its first upload, the export preflight was already sitting on
/// its limit, and every byte written afterwards (WAL, container logs, the compression backlog
/// draining at exactly that hour) pushed it under. The release would then COMMIT — event closed,
/// uploads locked, epoch bumped, `event-closed` fanned out to every phone — and only then would
/// both workers bail, with no second release possible.
#[test]
fn the_upload_gate_closes_before_the_export_preflight_would_refuse() {
for media_gb in [0u64, 1, 4, 8, 16, 32] {
let required = required_free_bytes(media_gb * GB, 2);
// `services::export::preflight` bails below this.
let preflight_refuses_below = required + DISK_RESERVE_BYTES as u64;
// `handlers::upload` refuses below this.
let gate_refuses_below = preflight_refuses_below + UPLOAD_GATE_HEADROOM_BYTES as u64;
assert!(
gate_refuses_below > preflight_refuses_below,
"at media={media_gb}GB the gate and the preflight share a threshold, so the \
keepsake's fate rests on whatever is written after uploads stop"
);
// At the instant the last upload is refused, the preflight must still pass with the
// whole headroom to spare — that is the slack the night's remaining writes consume.
let free_when_gate_closes = gate_refuses_below;
assert!(
free_when_gate_closes
>= preflight_refuses_below + UPLOAD_GATE_HEADROOM_BYTES as u64,
"at media={media_gb}GB there is no slack between the gate closing and the \
preflight failing"
);
}
}
#[test]
fn plenty_of_space_is_still_low_when_the_keepsake_would_not_fit() {
// THE case the fixed threshold misses, and the one that matters: 30 GB free is nowhere near
@@ -1051,7 +1157,7 @@ mod tests {
// size — see `disk_is_low`. Warning at the bare size fired only after the gate had
// already blocked every guest.
let required = 20 * GB;
let gate = required + DISK_RESERVE_BYTES as u64;
let gate = required + DISK_RESERVE_BYTES as u64 + UPLOAD_GATE_HEADROOM_BYTES as u64;
let warn_at = gate + gate / 4;
assert!(!disk_is_low(warn_at, required), "exactly enough is enough");
assert!(disk_is_low(warn_at - 1, required));
@@ -1060,8 +1166,9 @@ mod tests {
#[test]
fn an_empty_gallery_still_reserves_room_for_postgres() {
// With no gallery the keepsake term is 0, so the warn threshold collapses to
// 1.25 x DISK_RESERVE_BYTES (12.5 GB), which dominates the 10 GB absolute floor.
assert!(!disk_is_low(13 * GB, 0));
// 1.25 x (DISK_RESERVE_BYTES + UPLOAD_GATE_HEADROOM_BYTES) = 1.25 x 11.5 GB = 14.375 GB,
// which dominates the 10 GB absolute floor.
assert!(!disk_is_low(15 * GB, 0));
assert!(disk_is_low(9 * GB, 0));
}
}