diff --git a/backend/migrations/022_feed_scalar_counts.down.sql b/backend/migrations/022_feed_scalar_counts.down.sql new file mode 100644 index 0000000..207b02c --- /dev/null +++ b/backend/migrations/022_feed_scalar_counts.down.sql @@ -0,0 +1,26 @@ +-- Restore the 016 definition verbatim. +DROP VIEW IF EXISTS v_feed; +CREATE VIEW v_feed AS +SELECT + u.id, + u.event_id, + u.user_id, + usr.display_name AS uploader_name, + usr.is_banned, + usr.uploads_hidden, + u.preview_path, + u.thumbnail_path, + u.display_path, + u.mime_type, + u.caption, + u.created_at, + COUNT(DISTINCT l.user_id) AS like_count, + COUNT(DISTINCT c.id) AS comment_count +FROM upload u +JOIN "user" usr ON u.user_id = usr.id +LEFT JOIN "like" l ON l.upload_id = u.id +LEFT JOIN comment c ON c.upload_id = u.id AND c.deleted_at IS NULL +WHERE u.deleted_at IS NULL + AND usr.uploads_hidden = FALSE + AND usr.is_banned = FALSE +GROUP BY u.id, usr.display_name, usr.is_banned, usr.uploads_hidden; diff --git a/backend/migrations/022_feed_scalar_counts.up.sql b/backend/migrations/022_feed_scalar_counts.up.sql new file mode 100644 index 0000000..1ed7d61 --- /dev/null +++ b/backend/migrations/022_feed_scalar_counts.up.sql @@ -0,0 +1,55 @@ +-- Make a feed page cost a page, not the whole event. +-- +-- The previous definition (016) computed like_count/comment_count with LEFT JOINs and a +-- GROUP BY. Postgres CAN push the `event_id = $1` qual and the keyset predicate through the +-- view — verified with EXPLAIN, it uses idx_upload_event_created_id — but it CANNOT push +-- ORDER BY ... LIMIT across a GroupAggregate. So every feed request aggregated every upload in +-- the event (times its likes and comments) and only then sorted and took 21 rows. The cost +-- grew with the event, not with the page, and page 1 — the most expensive one — is exactly +-- what refreshFeedInPlace refetches on every completed upload, from every open feed in the +-- venue. +-- +-- Correlated scalar subqueries move the counts ABOVE the Limit in the plan: they are evaluated +-- once per returned row, so 21 index lookups instead of a full aggregation. +-- +-- The rewrite is EXACTLY equivalent, not merely close: +-- * "like" is keyed (upload_id, user_id), so COUNT(DISTINCT l.user_id) == count(*). +-- * comment.id is the primary key, so COUNT(DISTINCT c.id) == count(*). +-- * one row per upload either way — the GROUP BY was on u.id. +-- Column names, order and types are unchanged (count(*) and COUNT(DISTINCT ...) are both +-- bigint), so no Rust code changes. +-- +-- No new index needed: idx_like_upload plus the (upload_id, user_id) PK serve the like +-- subquery, and idx_comment_upload ... WHERE deleted_at IS NULL matches the comment +-- subquery's predicate exactly. +-- +-- One thing a future editor needs to know: the hashtag-filtered feed joins upload_hashtag +-- against this view. That was safe before only because the GROUP BY collapsed the join +-- fan-out; it is safe now because the view is one row per upload and upload_hashtag is keyed +-- (upload_id, hashtag_id) with a single tag filtered. Adding a second tag filter would need +-- fresh thought. + +-- Not CASCADE: if something ever comes to depend on this view, the migration should fail +-- loudly rather than silently drop it. +DROP VIEW IF EXISTS v_feed; +CREATE VIEW v_feed AS +SELECT + u.id, + u.event_id, + u.user_id, + usr.display_name AS uploader_name, + usr.is_banned, + usr.uploads_hidden, + u.preview_path, + u.thumbnail_path, + u.display_path, + u.mime_type, + u.caption, + u.created_at, + (SELECT count(*) FROM "like" l WHERE l.upload_id = u.id) AS like_count, + (SELECT count(*) FROM comment c WHERE c.upload_id = u.id AND c.deleted_at IS NULL) AS comment_count +FROM upload u +JOIN "user" usr ON u.user_id = usr.id +WHERE u.deleted_at IS NULL + AND usr.uploads_hidden = FALSE + AND usr.is_banned = FALSE; diff --git a/backend/src/db.rs b/backend/src/db.rs index 62ccb9e..b7f97ea 100644 --- a/backend/src/db.rs +++ b/backend/src/db.rs @@ -4,6 +4,15 @@ use sqlx::postgres::PgPoolOptions; const DEFAULT_MAX_CONNECTIONS: u32 = 10; +/// How long a request waits for a pool connection before being shed. +/// +/// sqlx's default is 30 s — longer than the frontend's own 20 s fetch timeout (api.ts +/// TIMEOUT_MS), so under saturation the browser gave up while the server kept holding the +/// slot: the client saw a timeout, the server saw a completed request, and the work was done +/// for nobody. Failing fast sheds load instead of compounding it, and `From` in +/// error.rs turns the timeout into a 503 with a Retry-After rather than a bare 500. +const ACQUIRE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(5); + /// SQLSTATE for `invalid_password`. const PG_INVALID_PASSWORD: &str = "28P01"; @@ -54,6 +63,7 @@ pub async fn create_pool(database_url: &str) -> Result { let pool = match PgPoolOptions::new() .max_connections(max_connections) + .acquire_timeout(ACQUIRE_TIMEOUT) .connect(database_url) .await { diff --git a/backend/src/error.rs b/backend/src/error.rs index 30fa2ad..d61a356 100644 --- a/backend/src/error.rs +++ b/backend/src/error.rs @@ -19,6 +19,12 @@ pub enum AppError { /// the client can treat it as *terminal* (413, no retry) instead of backing off and /// retrying a permanently-failing upload forever. QuotaExceeded(String), + /// The server is temporarily unable to serve this request — currently only pool + /// saturation. Distinct from `Internal` because it is TRANSIENT and the client should be + /// told so: a 500 reads as "this request is broken", while a 503 + Retry-After reads as + /// "come back shortly", which is what the upload queue's retry classifier needs to make + /// the right call. Second field: optional retry-after seconds. + ServiceUnavailable(String, Option), Internal(anyhow::Error), } @@ -33,6 +39,9 @@ impl AppError { Self::Conflict(_) => (StatusCode::CONFLICT, "conflict"), Self::TooManyRequests(..) => (StatusCode::TOO_MANY_REQUESTS, "too_many_requests"), Self::QuotaExceeded(_) => (StatusCode::PAYLOAD_TOO_LARGE, "quota_exceeded"), + Self::ServiceUnavailable(..) => { + (StatusCode::SERVICE_UNAVAILABLE, "service_unavailable") + } Self::Internal(_) => (StatusCode::INTERNAL_SERVER_ERROR, "internal_error"), } } @@ -46,6 +55,7 @@ impl AppError { | Self::NotFound(msg) | Self::Conflict(msg) => msg.clone(), Self::TooManyRequests(msg, _) => msg.clone(), + Self::ServiceUnavailable(msg, _) => msg.clone(), Self::QuotaExceeded(msg) => msg.clone(), Self::Internal(err) => { tracing::error!("internal error: {err:#}"); @@ -58,10 +68,13 @@ impl AppError { impl IntoResponse for AppError { fn into_response(self) -> Response { let (status, code) = self.status_and_code(); - let retry_after_secs = if let Self::TooManyRequests(_, Some(secs)) = &self { - Some(*secs) - } else { - None + // BOTH retry-carrying variants must be matched here. `message()` would fail to + // compile on a missing arm; this one would not — it would silently drop the header and + // the `retry_after_secs` body field, which is exactly the sort of omission that only + // shows up under the load the 503 exists for. + let retry_after_secs = match &self { + Self::TooManyRequests(_, secs) | Self::ServiceUnavailable(_, secs) => *secs, + _ => None, }; let message = self.message(); @@ -93,6 +106,84 @@ impl From for AppError { impl From for AppError { fn from(err: sqlx::Error) -> Self { - Self::Internal(err.into()) + match err { + // Pool saturation is load, not a bug. Reporting it as a 500 was actively harmful: + // the frontend's upload-queue classifier treats 5xx as transient and retries, so + // the retries piled straight back into the saturated pool with no Retry-After to + // pace them. A 503 says the same thing honestly and carries the backoff. + // + // `PoolClosed` stays `Internal` — it only happens during shutdown, where a 503 + // would invite a retry against a server that is going away. + sqlx::Error::PoolTimedOut => { + tracing::warn!("database pool exhausted; shedding a request with 503"); + Self::ServiceUnavailable( + "Server ist gerade ausgelastet. Bitte versuche es in ein paar Sekunden erneut." + .into(), + Some(POOL_TIMEOUT_RETRY_AFTER_SECS), + ) + } + other => Self::Internal(other.into()), + } + } +} + +/// Retry-After for a shed request. Short: pool saturation clears in seconds once the queue +/// drains, and a long value would make a brief spike feel like an outage. +const POOL_TIMEOUT_RETRY_AFTER_SECS: u64 = 3; + +#[cfg(test)] +mod tests { + use super::*; + + /// `into_response` extracts `retry_after_secs` by MATCHING ON VARIANTS, so unlike + /// `message()` a missing arm is not a compile error — it silently drops the header. Pin the + /// behaviour for both retry-carrying variants. + #[test] + fn both_retry_carrying_variants_emit_retry_after() { + for err in [ + AppError::TooManyRequests("slow down".into(), Some(42)), + AppError::ServiceUnavailable("busy".into(), Some(3)), + ] { + let expected = match &err { + AppError::TooManyRequests(_, Some(s)) | AppError::ServiceUnavailable(_, Some(s)) => { + s.to_string() + } + _ => unreachable!(), + }; + let resp = err.into_response(); + assert_eq!( + resp.headers() + .get(axum::http::header::RETRY_AFTER) + .and_then(|v| v.to_str().ok()), + Some(expected.as_str()), + "a shed/throttled client must be told when to come back" + ); + } + } + + /// Pool saturation is load, not a bug. A 500 makes the frontend's retry classifier pile + /// straight back into the saturated pool with no backoff to pace it. + #[test] + fn pool_exhaustion_sheds_with_503_but_shutdown_does_not() { + let shed: AppError = sqlx::Error::PoolTimedOut.into(); + assert_eq!( + shed.status_and_code(), + (StatusCode::SERVICE_UNAVAILABLE, "service_unavailable") + ); + + // PoolClosed only happens during shutdown; a 503 there would invite a retry against a + // server that is going away. + let closing: AppError = sqlx::Error::PoolClosed.into(); + assert_eq!( + closing.status_and_code(), + (StatusCode::INTERNAL_SERVER_ERROR, "internal_error") + ); + + // Everything else must keep its existing mapping. + let missing: AppError = sqlx::Error::RowNotFound.into(); + assert_eq!( + missing.status_and_code(), + (StatusCode::INTERNAL_SERVER_ERROR, "internal_error") + ); } } diff --git a/frontend/src/routes/feed/+page.svelte b/frontend/src/routes/feed/+page.svelte index b663aed..4421587 100644 --- a/frontend/src/routes/feed/+page.svelte +++ b/frontend/src/routes/feed/+page.svelte @@ -273,7 +273,23 @@ // A processed upload gains preview/thumbnail URLs. Coalesce bursts (bulk // uploads fire one per file) into a single in-place merge so the feed // neither hammers the server nor collapses to page 1 / loses scroll. - onSseEvent('upload-processed', () => scheduleInPlaceRefresh()), + onSseEvent('upload-processed', (data) => { + // Only refetch if this client actually SHOWS the card that changed. Without the + // membership test every open feed in the venue (~100 at a reception) fired a + // page-1 refetch for every completed upload — a self-inflicted thundering herd + // on the most expensive page, triggered by the most common event. + // + // Nothing is lost by skipping: a client that doesn't have the card also missed + // its `new-upload`, and the reconnect `feed-delta` below already calls + // scheduleInPlaceRefresh(). + try { + const { upload_id } = JSON.parse(data) as { upload_id: string }; + if (!uploads.some((u) => u.id === upload_id)) return; + } catch { + return; + } + scheduleInPlaceRefresh(); + }), onSseEvent('upload-deleted', (data) => { try { const payload = JSON.parse(data) as { upload_id: string }; @@ -399,12 +415,21 @@ // Debounced page-1 fetch that *merges* (updates existing cards in place, prepends // genuinely new ones) rather than replacing the array — preserves scroll and any // pages already loaded below the fold. + /** Debounce floor. */ + const IN_PLACE_REFRESH_MIN_MS = 800; + /** Jitter added on top. Every open feed receives the same SSE at the same instant, so a + * fixed delay just moves the herd 800 ms later instead of spreading it. */ + const IN_PLACE_REFRESH_JITTER_MS = 1200; + function scheduleInPlaceRefresh() { if (inPlaceRefreshTimer) return; - inPlaceRefreshTimer = setTimeout(() => { - inPlaceRefreshTimer = null; - void refreshFeedInPlace(); - }, 800); + inPlaceRefreshTimer = setTimeout( + () => { + inPlaceRefreshTimer = null; + void refreshFeedInPlace(); + }, + IN_PLACE_REFRESH_MIN_MS + Math.random() * IN_PLACE_REFRESH_JITTER_MS + ); } async function refreshFeedInPlace() {