fix(export): pin export workers to a live release epoch; make reopen atomic
Adversarial re-review ofc197b2cfound the previous fix both incomplete AND that it introduced a new stuck-state regression. Two independent reviewers flagged the latter. HIGH (data loss, still open after the last fix) — a worker could claim a LIVE seq after a reopen. `claim_job` took any `pending` row. If a reopen landed between `release_gallery`'s spawn and the worker's claim, the row was left pending at the *bumped* seq, so the worker claimed a CURRENT generation and spent minutes exporting a snapshot taken while the event was open and guests were still uploading. On the next re-release (which re-arms `export_released_at` BEFORE it bumps the seq) that worker's finalize and ready-flip both passed their guards, flipped ready on its stale snapshot, and made `enqueue_and_spawn_exports` skip regeneration — silently serving a keepsake missing every upload from the reopen window. The reopen seq-bump did not help: the worker's seq was not stale. Fix: `claim_job` now refuses to claim unless the event is still released. A worker's generation is therefore always born AND finalized inside a single live release epoch. The unclaimed pending row is harmless — the next release resets and re-spawns it. HIGH (regression I introduced inc197b2c) — `open_event`'s event-UPDATE and its export_job seq-bump were two autocommit statements. The first is exactly what re-enables `release_gallery`, so a re-release could slip between them: it spawns a FRESH worker at seq N+1, then our second statement bumps that live generation to N+2, orphaning the worker. Its seq-guarded finalize/fail/flip then all match nothing, the row is stranded `running` with no owner, and the host cannot retry ("bereits freigegeben") — only a process restart recovers it. Fix: both statements now run in one transaction, so the row lock on `event` serializes `release_gallery` behind the commit. LOW - The flip-lost path left its stale archive on disk (asymmetric with the finalize-lost path, which deletes it). If a host reopened and never re-released, no future generation would ever prune it. Now discards `out_path` and still sweeps older gens. - A reopen clears the ready flags server-side, but nothing told the clients: the nav and /export page kept advertising a keepsake that now 404s. Both now refresh on `event-opened` (a superseded worker deliberately emits no `export-progress`). Tests - New: a release broadcasts `export-progress` 100 for both types + one `export-available`. The export SSE had ZERO e2e coverage, and this round made the emit conditional — `/export/status` polling reads the job row, not the SSE, so a regression here would have left every other test green while the live UI silently stopped updating. - New: open ‖ release churn always converges to a consistent, downloadable keepsake. Honestly labelled a STRESS test, not a regression guard: verified it still passes against a deliberately non-transactional build even with ~90 concurrent pairs, so it does NOT cover the atomicity fix (that window is not reproducible out-of-process). - Verified the reopen-supersession test is non-vacuous by empirically reverting the seq-bump — it fails, as intended. Verified: backend 40 tests, svelte-check 0 errors, e2e typecheck clean, e2e 158 passed / 1 skipped on chromium-desktop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -515,6 +515,17 @@ pub async fn open_event(
|
||||
// release time, so allowing new uploads afterwards would silently diverge the live
|
||||
// feed from the frozen export. Clearing `export_released_at` (and the readiness
|
||||
// flags) lets the host re-release later to regenerate a correct, complete keepsake.
|
||||
//
|
||||
// BOTH statements run in ONE transaction. They must be atomic: the first makes
|
||||
// `release_gallery` possible again (it gates on `export_released_at IS NULL`), so if a
|
||||
// concurrent re-release could slip between them it would enqueue + spawn a FRESH worker at
|
||||
// seq N+1, and our second statement would then bump that brand-new generation to N+2 —
|
||||
// orphaning the live worker (its seq-guarded finalize/fail/flip all match nothing), stranding
|
||||
// the row at `running` with no owner, and leaving the host unable to retry ("bereits
|
||||
// freigegeben"). Inside a transaction, our row lock on `event` serializes `release_gallery`'s
|
||||
// claim behind our commit, so it only ever sees the fully-reopened, fully-bumped state.
|
||||
let mut tx = state.pool.begin().await?;
|
||||
|
||||
let result = sqlx::query(
|
||||
"UPDATE event
|
||||
SET uploads_locked_at = NULL,
|
||||
@@ -524,10 +535,12 @@ pub async fn open_event(
|
||||
WHERE slug = $1 AND (uploads_locked_at IS NOT NULL OR export_released_at IS NOT NULL)",
|
||||
)
|
||||
.bind(&state.config.event_slug)
|
||||
.execute(&state.pool)
|
||||
.execute(&mut *tx)
|
||||
.await?;
|
||||
|
||||
if result.rows_affected() > 0 {
|
||||
let reopened = result.rows_affected() > 0;
|
||||
|
||||
if reopened {
|
||||
// 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
|
||||
@@ -535,17 +548,20 @@ pub async fn open_event(
|
||||
// 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.
|
||||
// pre-reopen keepsake (silent data loss). Bumping here retires the old seq for good.
|
||||
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)
|
||||
.execute(&mut *tx)
|
||||
.await?;
|
||||
}
|
||||
|
||||
tx.commit().await?;
|
||||
|
||||
if reopened {
|
||||
let _ = state.sse_tx.send(SseEvent::new("event-opened", "{}"));
|
||||
}
|
||||
|
||||
|
||||
@@ -326,7 +326,14 @@ async fn run_zip_export_inner(
|
||||
.await?;
|
||||
|
||||
if flipped.rows_affected() == 0 {
|
||||
tracing::info!("ZIP export for event {event_id} superseded before ready-flip; not advertising");
|
||||
// Superseded between finalize and here. Discard our now-stale archive (mirroring the
|
||||
// finalize-lost branch above) instead of leaving it to linger: if the host reopens and
|
||||
// never re-releases, no future generation would ever come along to prune it. We still
|
||||
// sweep strictly-older generations — that's safe from any worker, and keeps a reopen that
|
||||
// is never followed by a re-release from stranding the whole export dir on disk.
|
||||
let _ = tokio::fs::remove_file(&out_path).await;
|
||||
prune_stale_export_files(&exports_dir, "Gallery", event_id, seq).await;
|
||||
tracing::info!("ZIP export for event {event_id} superseded before ready-flip; discarded");
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
@@ -674,7 +681,10 @@ async fn run_html_export_inner(
|
||||
.await?;
|
||||
|
||||
if flipped.rows_affected() == 0 {
|
||||
tracing::info!("HTML export for event {event_id} superseded before ready-flip; not advertising");
|
||||
// Superseded — discard our stale archive, still sweep older generations (see run_zip_export).
|
||||
let _ = tokio::fs::remove_file(&out_path).await;
|
||||
prune_stale_export_files(&exports_dir, "Memories", event_id, seq).await;
|
||||
tracing::info!("HTML export for event {event_id} superseded before ready-flip; discarded");
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
@@ -746,10 +756,25 @@ async fn query_hashtags(pool: &PgPool, event_id: Uuid) -> Result<Vec<(Uuid, Stri
|
||||
/// `status = 'pending'`. The caller threads the returned seq into per-generation temp/final
|
||||
/// paths and into the guarded finalize, so a superseded worker never corrupts or resurrects
|
||||
/// a stale keepsake.
|
||||
/// Claim the pending job for `(event, type)` and return the `release_seq` we own.
|
||||
///
|
||||
/// The `export_released_at IS NOT NULL` guard pins a worker to a LIVE release epoch. Without it,
|
||||
/// a reopen landing between `release_gallery`'s spawn and this claim would leave the row `pending`
|
||||
/// at the *bumped* seq, and this worker would then claim that seq — a CURRENT generation — and
|
||||
/// spend minutes exporting a snapshot taken while the event was open and guests were still
|
||||
/// uploading. On the next re-release (which re-arms `export_released_at` before it bumps the seq)
|
||||
/// that worker's finalize + ready-flip would both pass their guards, flip ready on its stale
|
||||
/// snapshot, and make `enqueue_and_spawn_exports` skip regeneration (`if ready { continue }`) —
|
||||
/// silently serving a keepsake missing every upload from the reopen window. Refusing the claim
|
||||
/// while the event is un-released closes that: a worker's generation is now always born AND
|
||||
/// finalized inside a single live release. The unclaimed `pending` row is harmless — the next
|
||||
/// release resets and re-spawns it.
|
||||
async fn claim_job(pool: &PgPool, event_id: Uuid, export_type: &str) -> Option<i64> {
|
||||
sqlx::query_as::<_, (i64,)>(
|
||||
"UPDATE export_job SET status = 'running'
|
||||
WHERE event_id = $1 AND type = $2::export_type AND status = 'pending'
|
||||
AND EXISTS (SELECT 1 FROM event
|
||||
WHERE id = $1 AND export_released_at IS NOT NULL)
|
||||
RETURNING release_seq",
|
||||
)
|
||||
.bind(event_id)
|
||||
|
||||
Reference in New Issue
Block a user