diff --git a/backend/src/handlers/host.rs b/backend/src/handlers/host.rs index 194788b..3f9dccb 100644 --- a/backend/src/handlers/host.rs +++ b/backend/src/handlers/host.rs @@ -528,6 +528,24 @@ pub async fn open_event( .await?; if result.rows_affected() > 0 { + // A reopen invalidates any released keepsake, so make it a *supersession point* for the + // export pipeline: bump every export_job generation for this event. An in-flight worker + // — or one that has already `finalize_job`'d but not yet flipped its ready flag — captured + // the pre-reopen `release_seq`, so its guarded finalize/ready-flip now match nothing and + // become no-ops. Without this, the `export_released_at IS NOT NULL` flip guard alone leaves + // a sub-window open: a re-release re-arms `export_released_at` *before* it bumps the seq, and + // a stale worker's flip landing in that gap satisfies both guards and resurrects a + // pre-reopen keepsake (silent data loss). Bumping here closes it — the old seq is retired + // for good and the next re-release regenerates from a current snapshot. + sqlx::query( + "UPDATE export_job SET release_seq = release_seq + 1 + FROM event e + WHERE e.id = export_job.event_id AND e.slug = $1", + ) + .bind(&state.config.event_slug) + .execute(&state.pool) + .await?; + let _ = state.sse_tx.send(SseEvent::new("event-opened", "{}")); } diff --git a/backend/src/services/export.rs b/backend/src/services/export.rs index 85d22e5..ec01b65 100644 --- a/backend/src/services/export.rs +++ b/backend/src/services/export.rs @@ -303,16 +303,16 @@ async fn run_zip_export_inner( return Ok(()); } - // Flip the ready flag only while still current AND still released. The `release_seq` - // EXISTS check alone is NOT enough: a reopen (`open_event`) landing in the window between - // our `finalize_job` above and this UPDATE clears `export_released_at` + both ready flags - // but leaves our `export_job` row `done` at this same seq — so EXISTS would still match and - // we'd resurrect `export_zip_ready = TRUE` on a keepsake that predates the reopen. The next - // re-release would then read that stale TRUE, skip regeneration (`if ready { continue }`), - // and serve a snapshot missing every upload added during the reopen window — the exact - // stale-keepsake data loss migration 012 exists to prevent. Anchoring on - // `export_released_at IS NOT NULL` makes the flip a no-op once a reopen has landed. - sqlx::query( + // Flip the ready flag only while our generation is still current AND the event is still + // released. Two layers guard this against a reopen resurrecting a pre-reopen keepsake: + // 1. `release_seq = $2` — a reopen bumps every export_job's seq (see `open_event`), so a + // stale worker holding the old seq matches nothing here. + // 2. `export_released_at IS NOT NULL` — belt-and-suspenders in case any path clears the + // release without bumping the seq. + // If this flip is a no-op (0 rows), a reopen/supersession has landed: the keepsake isn't + // downloadable and a fresh generation owns cleanup + the completion signal, so we must NOT + // advertise 100% or prune on this superseded generation's behalf. + let flipped = sqlx::query( "UPDATE event SET export_zip_ready = TRUE WHERE id = $1 AND export_released_at IS NOT NULL @@ -325,6 +325,11 @@ async fn run_zip_export_inner( .execute(pool) .await?; + if flipped.rows_affected() == 0 { + tracing::info!("ZIP export for event {event_id} superseded before ready-flip; not advertising"); + return Ok(()); + } + prune_stale_export_files(&exports_dir, "Gallery", event_id, seq).await; let _ = sse_tx.send(SseEvent { @@ -652,9 +657,10 @@ async fn run_html_export_inner( return Ok(()); } - // Same released-anchored guard as the ZIP flip (see run_zip_export): a reopen between our - // finalize and here must not resurrect `export_html_ready` on a pre-reopen snapshot. - sqlx::query( + // Same two-layer guard as the ZIP flip (see run_zip_export): the reopen seq-bump plus the + // `export_released_at IS NOT NULL` anchor keep a superseded worker from resurrecting + // `export_html_ready` on a pre-reopen snapshot. A no-op flip → superseded → don't advertise/prune. + let flipped = sqlx::query( "UPDATE event SET export_html_ready = TRUE WHERE id = $1 AND export_released_at IS NOT NULL @@ -667,6 +673,11 @@ async fn run_html_export_inner( .execute(pool) .await?; + if flipped.rows_affected() == 0 { + tracing::info!("HTML export for event {event_id} superseded before ready-flip; not advertising"); + return Ok(()); + } + prune_stale_export_files(&exports_dir, "Memories", event_id, seq).await; let _ = sse_tx.send(SseEvent { diff --git a/e2e/page-objects/host-dashboard.ts b/e2e/page-objects/host-dashboard.ts index b322072..991e1e1 100644 --- a/e2e/page-objects/host-dashboard.ts +++ b/e2e/page-objects/host-dashboard.ts @@ -25,12 +25,11 @@ export class HostDashboard { return this.page.locator('tr,li,div', { hasText: displayName }).filter({ has: this.page.getByRole('button') }).first(); } - async banUser(displayName: string, hideUploads = false) { + async banUser(displayName: string) { + // A ban always hides — the confirm dialog is a plain confirm, no "hide uploads" checkbox + // (removed from the UI; the endpoint takes no body). const row = this.userRow(displayName); await row.getByRole('button', { name: /sperren/i }).first().click(); - if (hideUploads) { - await this.page.getByRole('checkbox', { name: /ausblenden/i }).check(); - } await this.page.getByRole('button', { name: /bestätigen|sperren/i }).last().click(); } } diff --git a/e2e/specs/04-host/moderation.spec.ts b/e2e/specs/04-host/moderation.spec.ts index ba10ea4..6373c67 100644 --- a/e2e/specs/04-host/moderation.spec.ts +++ b/e2e/specs/04-host/moderation.spec.ts @@ -7,18 +7,9 @@ import { test, expect } from '../../fixtures/test'; import { seedUpload, seedComment } from '../../helpers/seed'; test.describe('Host — moderation API', () => { - test('ban with hide_uploads=true sets the right flags', async ({ api, host, guest }) => { - const target = await guest('Banned1'); - await api.banUser(host.jwt, target.userId); - const users = await api.listUsers(host.jwt); - const row = users.find((u: any) => u.id === target.userId); - expect(row?.is_banned).toBe(true); - expect(row?.uploads_hidden).toBe(true); - }); - - test('ban always hides — the hide_uploads flag is ignored (USER_JOURNEYS §10.5)', async ({ api, host, guest }) => { - // Ban is now unconditionally a hide: even asking NOT to hide still hides, because a - // banned user's content is "gone" everywhere. The legacy hide_uploads arg is ignored. + test('ban always hides — a banned user is is_banned AND uploads_hidden (USER_JOURNEYS §9.5)', async ({ api, host, guest }) => { + // Ban is unconditionally a hide: a banned user's content is "gone" everywhere. The endpoint + // takes no body and there is no per-request opt-out (the legacy hide_uploads flag is gone). const target = await guest('Banned2'); await api.banUser(host.jwt, target.userId); const users = await api.listUsers(host.jwt); diff --git a/e2e/specs/04-host/sse-eviction.spec.ts b/e2e/specs/04-host/sse-eviction.spec.ts index 463647a..c5eccfd 100644 --- a/e2e/specs/04-host/sse-eviction.spec.ts +++ b/e2e/specs/04-host/sse-eviction.spec.ts @@ -30,7 +30,7 @@ test.describe('Host — live SSE eviction (H3)', () => { ); }); - test('banning a user with hide_uploads broadcasts user-hidden', async ({ + test('banning a user broadcasts user-hidden', async ({ api, host, guest, diff --git a/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts b/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts index 705d05f..6028a5f 100644 --- a/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts +++ b/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts @@ -18,23 +18,23 @@ * download follows the current `done` row's `file_path`, never a fixed name. * * A second, narrower window on the SAME class of bug (fixed alongside): a reopen (`open_event`) - * landing between a *current* worker's `finalize_job` and its ready-flag flip. `open_event` - * clears `export_released_at` + both ready flags but leaves the `export_job` row `done` at the - * current seq — so the seq-guarded flip would still match and resurrect `export_zip_ready=TRUE` - * on a pre-reopen snapshot, and the next re-release would `if ready { continue }` and skip - * regeneration. The flip UPDATEs are therefore additionally anchored on - * `export_released_at IS NOT NULL`, making them a no-op once a reopen has landed. The - * "completeness" test below exercises the real reopen→re-release worker path end-to-end; the - * sub-millisecond finalize↔flip interleave itself isn't deterministically forceable with the - * fast fixtures, so that exact window is covered by the SQL guard + code review rather than a - * timing-dependent assertion. + * landing between a *current* worker's `finalize_job` and its ready-flag flip could resurrect + * `export_zip_ready=TRUE` on a pre-reopen snapshot, and the next re-release would + * `if ready { continue }` and skip regeneration. Two layers close it: (1) `open_event` now bumps + * every `export_job.release_seq`, so a reopen is itself a supersession point — a stale worker's + * captured seq no longer matches its `release_seq`-guarded flip; (2) the flip UPDATEs are also + * anchored on `export_released_at IS NOT NULL` as belt-and-suspenders. Layer (1) is what defeats + * the double-race where a re-release re-arms `export_released_at` before it bumps the seq. * * Coverage: * - churn integrity: rapid reopen→re-release yields exactly one INTACT ZIP, no stuck jobs. * - completeness: an upload added during the reopen window IS present in the re-released * keepsake (the direct data-loss regression). - * - supersession: a re-release SUPERSEDES an in-flight (running) job — it bumps the + * - supersession (re-release): a re-release SUPERSEDES an in-flight (running) job — it bumps the * generation and regenerates, rather than leaving the stale worker to finalize. + * - supersession (reopen): a reopen ALONE bumps the generation, so a worker holding the + * pre-reopen seq can never flip a stale ready flag (closes the finalize↔flip double-race + * deterministically, without depending on sub-millisecond worker timing). */ import { test, expect } from '../../fixtures/test'; import { Client } from 'pg'; @@ -209,7 +209,8 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k WHERE e.id = ej.event_id AND e.slug = '${SLUG}' AND ej.type = 'zip'` ); - // Reopen (clears release + ready flags; does NOT touch job rows) then re-release. + // Reopen (clears release + ready flags AND bumps every export_job's release_seq) then + // re-release (bumps + resets again). Either bump alone supersedes the pinned running row. expect((await post('/api/v1/host/event/open', host.jwt)).status).toBe(204); expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204); @@ -227,4 +228,41 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k seqBefore ); }); + + test('a reopen ALONE bumps release_seq — a pre-reopen worker can never flip a stale ready flag', async ({ + host, + }) => { + // Deterministic proof of the reopen-supersession that closes the finalize↔flip double-race. + // The nasty interleaving (worker finalized `done@S`, THEN reopen, THEN a re-release re-arms + // `export_released_at` BEFORE it bumps the seq, THEN the stale worker's flip runs and both + // guards pass) can't be forced by sub-millisecond timing — but its ROOT cause is observable: + // whether a reopen retires the current generation's seq. If it does, the seq the worker + // captured (S) is gone for good, so its `release_seq = S`-guarded flip matches nothing no + // matter when it lands. We assert exactly that invariant. + await seedUpload(host.jwt, { caption: 'x' }); + expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204); + await waitExportDone(host.jwt); + const seqAtRelease = await zipReleaseSeq(); + + // Reopen only — no re-release. The export_job row is left `done` at the OLD seq by the old + // code; the fix bumps it here. + expect((await post('/api/v1/host/event/open', host.jwt)).status).toBe(204); + + const seqAfterReopen = await zipReleaseSeq(); + expect( + seqAfterReopen, + 'reopen must bump export_job.release_seq so a pre-reopen worker (seq=' + + seqAtRelease + + ') is superseded and can never flip a stale ready flag' + ).toBeGreaterThan(seqAtRelease); + + // And the reopen must have cleared readiness (belt-and-suspenders: the stale worker also + // can't satisfy `export_released_at IS NOT NULL`). + const [ev] = await pgQuery<{ zip_ready: boolean; released_at: string | null }>( + `SELECT export_zip_ready AS zip_ready, export_released_at AS released_at + FROM event WHERE slug = '${SLUG}'` + ); + expect(ev.zip_ready, 'reopen clears zip readiness').toBe(false); + expect(ev.released_at, 'reopen clears the release timestamp').toBeNull(); + }); });