test: replace coverage that could not fail with coverage that can

`backend/tests/` follows a house rule of copying production SQL character-for-
character rather than calling `src/`, because the crate is a binary and nothing
in it is importable from an integration test. For pinning behaviour that already
existed that is a defensible trade. Applied to a NEW fix whose only coverage is
the copy, it proves nothing: the fix and its test become two independent
implementations, and deleting the fix leaves the test green.

`audit_names.rs` did exactly that. It never called `audit::record` — it
reimplemented `resolve_names` and the INSERT inside the test file, down to a
hardcoded `.bind("host")`, and then asserted `actor_role == "host"` against its
own literal. That assertion could not fail for any change to the code it named,
and grep confirmed there was no other coverage of the audit-name work anywhere.

Moved into `#[cfg(test)]` inside `services/audit.rs`, where the real function IS
callable. CI already runs `cargo test --all-features` with a live DATABASE_URL,
so `#[sqlx::test]` works there; verified all four run and pass. The role
assertion now compares against `UserRole::as_str()` itself rather than a literal,
so it tracks a rename instead of pretending to, plus an explicit `assert_ne!`
against the Debug spelling.

Also:

- `retry-after-release.spec.ts` filtered the feed on `u.id === original.id` to
  prove "no second row was created". A duplicate gets a fresh uuid and could
  never match, so the filter yielded exactly 1 whether the gallery held one copy
  or five. Counts by uploader now, with the original's identity asserted
  separately. (The rest of that spec is sound — its 403 control and replay-id
  check both fail if the header fast-path is reverted.)

- `upload_after_release_commits_sees_the_lock_and_is_rejected` claimed the
  handler answers `UploadsLocked`. It answers `GalleryReleased` since the check
  order was inverted on this branch, and the test asserts no variant at all.
  Documented what it actually covers (the locked READ) and where the ordering IS
  covered (two e2e specs).

- Two `// SRC:` pointers had drifted ~130 lines into unrelated code, which is how
  a hand-copied fixture silently stops matching its original. Now named, not
  numbered.

- `emptyOutDir: false` claimed a failed viewer build "leaves the last good
  artifact in place". True for the `generateBundle` error, false for the newer
  `writeBundle` assertion, which fires after Vite has already written the file.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

backend/src/services/audit.rs

@@ -139,3 +139,176 @@ async fn resolve_names(
             .or_else(|| target_id.and_then(lookup)),
     )
 }
+
+/// These live HERE, not in `tests/`, and that is the entire point.
+///
+/// `backend/` is a binary crate, so an integration test cannot import `record`. The house rule in
+/// `tests/common/mod.rs` — copy the production SQL character-for-character — works for pinning
+/// behaviour that already existed, but applied to a NEW fix whose only coverage is the copy it
+/// proves nothing: the fix and its test become two independent implementations, and deleting the
+/// fix leaves the test green. The previous `tests/audit_names.rs` did exactly that, down to
+/// asserting `actor_role == "host"` against its own hardcoded `.bind("host")` — an assertion that
+/// could not fail for any change to the code it named.
+///
+/// A `#[cfg(test)]` module inside the binary can call the real function, so these do.
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    async fn seed_event(pool: &PgPool, slug: &str) -> Uuid {
+        sqlx::query_scalar("INSERT INTO event (slug, name) VALUES ($1, $2) RETURNING id")
+            .bind(slug)
+            .bind("Hochzeit")
+            .fetch_one(pool)
+            .await
+            .expect("seed event")
+    }
+
+    async fn seed_user(pool: &PgPool, event_id: Uuid, name: &str) -> Uuid {
+        sqlx::query_scalar(
+            "INSERT INTO \"user\" (event_id, display_name, recovery_pin_hash)
+             VALUES ($1, $2, 'x') RETURNING id",
+        )
+        .bind(event_id)
+        .bind(name)
+        .fetch_one(pool)
+        .await
+        .expect("seed user")
+    }
+
+    async fn audit_row(pool: &PgPool, action: &str) -> Option<(Option<String>, Option<String>)> {
+        sqlx::query_as(
+            "SELECT actor_name, target_name FROM host_action_audit
+              WHERE action = $1 ORDER BY created_at DESC LIMIT 1",
+        )
+        .bind(action)
+        .fetch_optional(pool)
+        .await
+        .expect("audit lookup")
+    }
+
+    /// The ordinary case: the caller supplies no names and `record` resolves both from the ids.
+    /// This is what nine of the eleven call sites do. Revert `resolve_names` and both names go NULL.
+    #[sqlx::test]
+    async fn a_recorded_action_carries_both_names_without_the_caller_supplying_them(pool: PgPool) {
+        let event_id = seed_event(&pool, "wedding").await;
+        let host = seed_user(&pool, event_id, "Gastgeberin Greta").await;
+        let guest = seed_user(&pool, event_id, "Gesperrter Gustav").await;
+
+        record(
+            &pool,
+            event_id,
+            host,
+            None,
+            UserRole::Host,
+            "ban_user",
+            Some(guest),
+            None,
+            None,
+        )
+        .await;
+
+        let (actor_name, target_name) = audit_row(&pool, "ban_user").await.expect("a row");
+        assert_eq!(actor_name.as_deref(), Some("Gastgeberin Greta"));
+        assert_eq!(target_name.as_deref(), Some("Gesperrter Gustav"));
+    }
+
+    /// `as_str()`, not the `Debug` spelling. Asserted against `UserRole::as_str` itself rather than
+    /// a literal, so it tracks a rename instead of pretending to: what must hold is that the column
+    /// carries the SAME string the rest of the codebase uses, whatever that string is. Swap line 75
+    /// back to `format!("{actor_role:?}")` and this goes red on the `Host`/`host` casing.
+    #[sqlx::test]
+    async fn the_role_column_carries_the_canonical_spelling(pool: PgPool) {
+        let event_id = seed_event(&pool, "wedding").await;
+        let host = seed_user(&pool, event_id, "Gastgeberin Greta").await;
+
+        record(
+            &pool,
+            event_id,
+            host,
+            None,
+            UserRole::Host,
+            "release_gallery",
+            None,
+            None,
+            None,
+        )
+        .await;
+
+        let role: String = sqlx::query_scalar(
+            "SELECT actor_role FROM host_action_audit WHERE action = 'release_gallery'",
+        )
+        .fetch_one(&pool)
+        .await
+        .expect("role");
+        assert_eq!(role, UserRole::Host.as_str());
+        assert_ne!(
+            role,
+            format!("{:?}", UserRole::Host),
+            "the Debug spelling is not a wire format"
+        );
+    }
+
+    /// The case the columns exist for. `delete_account` hard-deletes the user row, so a name
+    /// resolved AFTER the fact would be NULL — the caller has to pass it in.
+    #[sqlx::test]
+    async fn a_name_supplied_by_the_caller_survives_the_row_being_deleted(pool: PgPool) {
+        let event_id = seed_event(&pool, "wedding").await;
+        let leaver = seed_user(&pool, event_id, "Abschied Anke").await;
+
+        // Exactly the order `me::delete_account` runs in: the row goes first, the audit row second.
+        sqlx::query("DELETE FROM \"user\" WHERE id = $1")
+            .bind(leaver)
+            .execute(&pool)
+            .await
+            .expect("delete user");
+
+        record(
+            &pool,
+            event_id,
+            leaver,
+            Some("Abschied Anke"),
+            UserRole::Guest,
+            "delete_account",
+            Some(leaver),
+            Some("Abschied Anke"),
+            None,
+        )
+        .await;
+
+        let (actor_name, target_name) = audit_row(&pool, "delete_account").await.expect("a row");
+        assert_eq!(
+            actor_name.as_deref(),
+            Some("Abschied Anke"),
+            "the audit row must name the deleted account — resolving it later is impossible"
+        );
+        assert_eq!(target_name.as_deref(), Some("Abschied Anke"));
+    }
+
+    /// And the failure mode that made this worth testing: with nothing supplied and nothing to look
+    /// up, the write must still succeed (an audit row must never fail an action) and carry NULLs.
+    #[sqlx::test]
+    async fn an_unresolvable_name_writes_the_row_anyway(pool: PgPool) {
+        let event_id = seed_event(&pool, "wedding").await;
+        let ghost = Uuid::new_v4();
+
+        record(
+            &pool,
+            event_id,
+            ghost,
+            None,
+            UserRole::Host,
+            "reset_pin",
+            Some(ghost),
+            None,
+            None,
+        )
+        .await;
+
+        let (actor_name, target_name) = audit_row(&pool, "reset_pin")
+            .await
+            .expect("the row must be written even when no name can be resolved");
+        assert_eq!(actor_name, None);
+        assert_eq!(target_name, None);
+    }
+}

backend/tests/audit_names.rs

@@ -1,163 +0,0 @@
-//! The host action audit must stay READABLE, which means the names must actually be there.
-//!
-//! Migration 029 made `actor_id`/`target_id` non-FK on the stated grounds that "the record must
-//! survive the actor's account being removed, which is exactly when it is most likely to be
-//! wanted". Every call site passed `None` for both name columns, so what survived a deletion was a
-//! bare uuid resolving to nothing — the guarantee, minus the only thing that made it useful.
-//!
-//! A NULL name fails silently: the row is written, the action succeeds, and nobody finds out until
-//! a host is trying to answer "what happened to my photo?" the next morning. So it is asserted.
-
-mod common;
-
-use common::*;
-use sqlx::PgPool;
-use uuid::Uuid;
-
-async fn audit_row(
-    pool: &PgPool,
-    action: &str,
-) -> Option<(Option<String>, Option<String>, String)> {
-    sqlx::query_as(
-        "SELECT actor_name, target_name, actor_role FROM host_action_audit
-          WHERE action = $1 ORDER BY created_at DESC LIMIT 1",
-    )
-    .bind(action)
-    .fetch_optional(pool)
-    .await
-    .expect("audit lookup")
-}
-
-/// The ordinary case: the caller supplies no names and `record` resolves both from the ids.
-/// This is what nine of the eleven call sites do.
-#[sqlx::test]
-async fn a_recorded_action_carries_both_names_without_the_caller_supplying_them(pool: PgPool) {
-    let event_id = seed_event(&pool, "wedding").await;
-    let host = seed_user(&pool, event_id, "Gastgeberin Greta").await;
-    let guest = seed_user(&pool, event_id, "Gesperrter Gustav").await;
-
-    eventsnap_backend_audit_record(&pool, event_id, host, None, "ban_user", Some(guest), None)
-        .await;
-
-    let (actor_name, target_name, actor_role) = audit_row(&pool, "ban_user")
-        .await
-        .expect("a row was written");
-    assert_eq!(actor_name.as_deref(), Some("Gastgeberin Greta"));
-    assert_eq!(target_name.as_deref(), Some("Gesperrter Gustav"));
-    // `as_str()`, not the Debug spelling — a renamed variant must not silently change the column.
-    assert_eq!(actor_role, "host");
-}
-
-/// The case the columns exist for. `delete_account` hard-deletes the user row, so a name resolved
-/// AFTER the fact would be NULL — the caller has to pass it in, and this is what proves it does.
-#[sqlx::test]
-async fn a_name_supplied_by_the_caller_survives_the_row_being_deleted(pool: PgPool) {
-    let event_id = seed_event(&pool, "wedding").await;
-    let leaver = seed_user(&pool, event_id, "Abschied Anke").await;
-
-    // Exactly the order `me::delete_account` runs in: the row goes first, the audit row second.
-    sqlx::query("DELETE FROM \"user\" WHERE id = $1")
-        .bind(leaver)
-        .execute(&pool)
-        .await
-        .expect("delete user");
-
-    eventsnap_backend_audit_record(
-        &pool,
-        event_id,
-        leaver,
-        Some("Abschied Anke"),
-        "delete_account",
-        Some(leaver),
-        Some("Abschied Anke"),
-    )
-    .await;
-
-    let (actor_name, target_name, _) = audit_row(&pool, "delete_account")
-        .await
-        .expect("a row was written");
-    assert_eq!(
-        actor_name.as_deref(),
-        Some("Abschied Anke"),
-        "the audit row must name the deleted account — resolving it later is impossible"
-    );
-    assert_eq!(target_name.as_deref(), Some("Abschied Anke"));
-}
-
-/// And the failure mode that made this worth testing: with nothing supplied and nothing to look up,
-/// the write must still succeed (an audit row must never fail an action) and simply carry NULLs.
-#[sqlx::test]
-async fn an_unresolvable_name_writes_the_row_anyway(pool: PgPool) {
-    let event_id = seed_event(&pool, "wedding").await;
-    let ghost = Uuid::new_v4();
-
-    eventsnap_backend_audit_record(&pool, event_id, ghost, None, "reset_pin", Some(ghost), None)
-        .await;
-
-    let (actor_name, target_name, _) = audit_row(&pool, "reset_pin")
-        .await
-        .expect("the row must be written even when no name can be resolved");
-    assert_eq!(actor_name, None);
-    assert_eq!(target_name, None);
-}
-
-/// SRC: `services/audit.rs::record` — the resolution + insert, verbatim, since the crate is a
-/// binary and the function is not importable from an integration test.
-async fn eventsnap_backend_audit_record(
-    pool: &PgPool,
-    event_id: Uuid,
-    actor_id: Uuid,
-    actor_name: Option<&str>,
-    action: &str,
-    target_id: Option<Uuid>,
-    target_name: Option<&str>,
-) {
-    let need_actor = actor_name.is_none();
-    let need_target = target_name.is_none() && target_id.is_some();
-    let (actor_name, target_name) = if !need_actor && !need_target {
-        (
-            actor_name.map(str::to_owned),
-            target_name.map(str::to_owned),
-        )
-    } else {
-        let mut wanted: Vec<Uuid> = Vec::with_capacity(2);
-        if need_actor {
-            wanted.push(actor_id);
-        }
-        if let Some(t) = target_id
-            && need_target
-        {
-            wanted.push(t);
-        }
-        let rows: Vec<(Uuid, String)> =
-            sqlx::query_as("SELECT id, display_name FROM \"user\" WHERE id = ANY($1)")
-                .bind(&wanted)
-                .fetch_all(pool)
-                .await
-                .unwrap_or_default();
-        let lookup = |id: Uuid| rows.iter().find(|(i, _)| *i == id).map(|(_, n)| n.clone());
-        (
-            actor_name.map(str::to_owned).or_else(|| lookup(actor_id)),
-            target_name
-                .map(str::to_owned)
-                .or_else(|| target_id.and_then(lookup)),
-        )
-    };
-
-    sqlx::query(
-        "INSERT INTO host_action_audit
-           (event_id, actor_id, actor_name, actor_role, action, target_id, target_name, detail)
-         VALUES ($1, $2, $3, $4, $5, $6, $7, $8)",
-    )
-    .bind(event_id)
-    .bind(actor_id)
-    .bind(actor_name.as_deref())
-    .bind("host")
-    .bind(action)
-    .bind(target_id)
-    .bind(target_name.as_deref())
-    .bind(Option::<serde_json::Value>::None)
-    .execute(pool)
-    .await
-    .expect("audit insert must not fail");
-}

backend/tests/upload_concurrency.rs

@@ -15,7 +15,9 @@ use common::*;
 use sqlx::PgPool;
 use uuid::Uuid;
 
-/// SRC: `handlers/upload.rs:313-322` — the guarded quota increment, verbatim.
+/// SRC: `handlers/upload.rs::create_upload` — the guarded quota increment, verbatim.
+/// (Named, not line-numbered: the previous pointer drifted by ~130 lines and landed in unrelated
+/// code, which is how a hand-copied fixture silently stops matching its original.)
 /// Returns `rows_affected()`; the handler aborts the whole upload tx when this is 0.
 async fn quota_inc(exec: impl sqlx::PgExecutor<'_>, user_id: Uuid, size: i64, limit: i64) -> u64 {
     sqlx::query(
@@ -146,7 +148,8 @@ async fn quota_guard_is_atomic_under_concurrent_transactions(pool: PgPool) {
 // 6. The `FOR SHARE` upload lock vs. the release
 // ─────────────────────────────────────────────────────────────────────────────
 
-/// SRC: `handlers/upload.rs:297-303` — the in-transaction re-check under a row lock, verbatim.
+/// SRC: `handlers/upload.rs::create_upload` — the in-transaction `FOR SHARE` re-check, verbatim.
+/// (Named, not line-numbered — see the note on `quota_inc`.)
 async fn lock_and_read_event(
     tx: &mut sqlx::PgConnection,
     event_id: Uuid,
@@ -254,12 +257,19 @@ async fn for_share_upload_lock_serializes_against_release(pool: PgPool) {
 }
 
 /// The other side of the same lock: once the release has COMMITTED, the next upload's `FOR SHARE`
-/// re-read sees `export_released_at` set and the handler rejects it with `UploadsLocked`.
+/// re-read sees `export_released_at` set and the handler bails out.
 ///
 /// PREVENTS: the same lost photo, on the losing side of the race — a photo committing AFTER the
 /// export snapshot would be in the live feed but missing from the keepsake. Rejecting is the correct
-/// outcome, and it is reversible: `UploadsLocked` (not Forbidden) tells the client to keep the blob
+/// outcome, and it is reversible: the client keeps the blob and resumes when the host reopens.
-/// and resume when the host reopens.
+///
+/// SCOPE, because the name overstates it: this asserts only what the LOCKED READ observes. It does
+/// not go through the handler, so it says nothing about which error the handler picks. That
+/// distinction is load-bearing — `create_upload` answers a released gallery with `GalleryReleased`
+/// and a plain lock with `UploadsLocked`, in that order, and the two drive different client
+/// behaviour (a `reopen` park vs. a retry). The ordering is covered end-to-end by
+/// `e2e/specs/10-flow-review/upload-lock-code.spec.ts` and `02-upload/retry-after-release.spec.ts`;
+/// this test's doc used to claim `UploadsLocked` outright and was simply wrong after that split.
 #[sqlx::test]
 async fn upload_after_release_commits_sees_the_lock_and_is_rejected(pool: PgPool) {
     let event_id = seed_event(&pool, "wedding").await;

e2e/specs/02-upload/retry-after-release.spec.ts

@@ -75,11 +75,21 @@ test.describe('Upload — a retry after release replays instead of refusing', ()
     );
 
     // 4. And no second row was created — the whole point of the key.
+    //
+    // Counted by UPLOADER, not by `id`. Filtering on `u.id === original.id` looks like a duplicate
+    // check and is not one: a duplicate row gets a fresh uuid, so it could never match, and the
+    // filter yields exactly 1 whether the gallery holds one copy or five. This guest uploaded once
+    // successfully ('a.jpg'); 'b.jpg' was refused at step 2 and the retry must have replayed rather
+    // than stored, so their total must be exactly one.
     const feed = await fetch(`${BASE}/api/v1/feed?limit=100`, {
       headers: { Authorization: `Bearer ${g.jwt}` },
     });
     const items: any[] = (await feed.json()).uploads ?? [];
-    const mine = items.filter((u) => u.id === original.id);
+    const mine = items.filter((u) => u.user_id === g.userId);
-    expect(mine.length, 'the photo must appear exactly once').toBe(1);
+    expect(
+      mine.length,
+      `the retry must not have stored a second copy; got ${mine.map((u) => u.id).join(', ')}`
+    ).toBe(1);
+    expect(mine[0].id, 'and the one that exists is the original').toBe(original.id);
   });
 });

frontend/export-viewer/vite.standalone.config.js

@@ -115,8 +115,17 @@ export default defineConfig({
 		// viewer at all. Before the guard existed the build could not fail, so neither could this.
 		//
 		// The only output is a single `index.html`, overwritten on every successful build, so
-		// there is nothing to accumulate — and a failed build now leaves the last good artifact in
+		// there is nothing to accumulate.
-		// place instead of deleting it.
+		//
+		// This protects ONE of the two failure paths, not both. `inlineThemeFonts` errors from
+		// `generateBundle`, before anything is written, so the previous artifact survives intact.
+		// The external-asset assertion errors from `writeBundle` — which runs AFTER Vite has
+		// written `index.html` — so that failure does overwrite the good viewer with the broken
+		// one. It cannot ship: the build exits non-zero, and `the_keepsake_viewer_is_compiled_into_
+		// this_binary` plus the `!html.contains("url(/")` assertion both fail the Rust test suite
+		// before any image is built. But if a `writeBundle` failure is what you are looking at,
+		// restore the artifact with `git checkout backend/static/export-viewer/` rather than
+		// assuming the working copy is still the last good one.
 		emptyOutDir: false,
 		target: 'es2020'
 	}