From 2bef6e19ef3d90c7a135b0023284c93c3bba7b35 Mon Sep 17 00:00:00 2001 From: fabi Date: Tue, 14 Jul 2026 22:50:17 +0200 Subject: [PATCH] fix(export): deleting a comment mid-build no longer strands the ZIP forever MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while auditing the test suite: the ViewerOnly carry-forward (added by me in 9666d74) permanently breaks the ZIP download if a comment is deleted while the ZIP is still being built. A comment-only change doesn't alter the ZIP's media, so `Affects::ViewerOnly` carries the finished archive into the new epoch rather than rebuilding it. But "finished" is a PRECONDITION, and between `release_gallery` and the ZIP worker completing it is false — for a real multi-GB gallery, for MINUTES. Deleting a comment in that window is an utterly ordinary thing to do. The carry-forward UPDATE is guarded on `status = 'done'`, so in that window it matched nothing — and ViewerOnly re-armed only the viewer. The ZIP row was left at the retired epoch; the in-flight worker finished and wrote `done` at an epoch `export_current` no longer matches. `GET /export/zip` then 404s FOREVER: recovery only runs at boot, and `release_gallery` refuses an already-released event. The guests' keepsake is simply gone, and the only escape is the rebuild button added one commit ago. Fix: the carry-forward's own `rows_affected()` decides. It matched => there is a current, finished ZIP and only the viewer needs rebuilding. It didn't => there is nothing to preserve, so rebuild the ZIP too, like any other invalidation. Never assume the precondition; ask the UPDATE. Proven non-vacuous: with the fix reverted the new test fails with the ZIP stranded at the retired epoch (job epoch 1, event epoch 2, export_current holding only "html", download 404). Backend 40 tests, e2e 163 passed / 1 skipped. Co-Authored-By: Claude Opus 4.8 --- backend/src/services/export.rs | 36 ++++++++----- .../export-reopen-rerelease.spec.ts | 54 +++++++++++++++++++ 2 files changed, 78 insertions(+), 12 deletions(-) diff --git a/backend/src/services/export.rs b/backend/src/services/export.rs index f212dc8..dba46bb 100644 --- a/backend/src/services/export.rs +++ b/backend/src/services/export.rs @@ -183,12 +183,21 @@ pub async fn invalidate_and_arm( return Ok(None); }; - if affects == Affects::ViewerOnly { - // Carry the finished ZIP into the new epoch instead of rebuilding it: same file, same - // `file_path`, now stamped current so `export_current` keeps serving it. Only the viewer is - // re-armed below. (`prune_stale_export_files` is told to protect any file a current-epoch - // row still points at, so the carried archive isn't swept for having an older epoch in its - // name.) + // A comment-only change doesn't alter the ZIP's contents (the ZIP holds media, not comments), so + // we'd rather carry the finished archive into the new epoch than spend minutes rebuilding it. + // + // But "finished" is the whole precondition, and it is NOT guaranteed: between `release_gallery` + // and the ZIP worker completing, the row sits at `pending`/`running` — MINUTES, for a real + // multi-GB gallery — and deleting a comment right after release is an utterly ordinary thing to + // do. If we blindly re-armed only the viewer, the carry-forward would match nothing, the ZIP row + // would be left stranded at the retired epoch, and NOTHING would ever re-arm it: the in-flight + // worker finishes and writes `done` at an epoch `export_current` no longer matches, so + // `GET /export/zip` 404s forever (short of a boot or the host finding the rebuild button). + // + // So the carry-forward's OWN result decides. It matched ⇒ there is a current, finished ZIP and + // only the viewer needs rebuilding. It didn't ⇒ there is no ZIP to preserve, and the ZIP must be + // rebuilt at the new epoch like any other invalidation. Never assume; ask the UPDATE. + let carried = if affects == Affects::ViewerOnly { sqlx::query( "UPDATE export_job SET epoch = $2 WHERE event_id = $1 AND type = 'zip'::export_type @@ -197,13 +206,16 @@ pub async fn invalidate_and_arm( .bind(event_id) .bind(epoch) .execute(&mut *conn) - .await?; - } - - let types: &[&str] = match affects { - Affects::Both => &["zip", "html"], - Affects::ViewerOnly => &["html"], + .await? + .rows_affected() + == 1 + } else { + false }; + + // (`prune_stale_export_files` protects any file a current-epoch row still points at, so a + // carried archive isn't swept for having an older epoch in its name.) + let types: &[&str] = if carried { &["html"] } else { &["zip", "html"] }; enqueue_types_at_epoch(&mut *conn, event_id, epoch, types).await?; Ok(Some(PendingRegen { event_id, event_name, epoch })) 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 f9df4fa..6539025 100644 --- a/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts +++ b/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts @@ -421,6 +421,60 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k expect(listing.some((n) => n.includes(keep)), 'the kept photo survives').toBe(true); }); + test('a comment deleted while the ZIP is still BUILDING does not strand the ZIP', async ({ + host, + guest, + }) => { + // A comment-only change (`Affects::ViewerOnly`) carries the finished ZIP into the new epoch + // rather than spending minutes rebuilding an archive whose media didn't change. But "finished" + // is a PRECONDITION, and between `release_gallery` and the ZIP worker completing it is false — + // for a real multi-GB gallery, for MINUTES. Deleting a comment in that window is an utterly + // ordinary thing to do. + // + // The carry-forward UPDATE is guarded on `status = 'done'`, so in that window it matched + // NOTHING — and ViewerOnly re-armed only the viewer. The ZIP row was left at the retired epoch, + // the in-flight worker finished and wrote `done` at an epoch `export_current` no longer matches, + // and `GET /export/zip` 404'd FOREVER. Nothing re-armed it: recovery only runs at boot, and + // `release_gallery` refuses an already-released event. + // + // Fix: the carry-forward's own `rows_affected()` decides. Nothing carried ⇒ rebuild the ZIP too. + const g = await guest('Commenter'); + const uploadId = await seedUpload(host.jwt, { caption: 'pic' }); + const cRes = await fetch(BASE + `/api/v1/upload/${uploadId}/comments`, { + method: 'POST', + headers: { Authorization: `Bearer ${g.jwt}`, 'Content-Type': 'application/json' }, + body: JSON.stringify({ body: 'schoenes foto' }), + }); + expect(cRes.status).toBe(201); + const commentId = (await cRes.json()).id; + + expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204); + await waitExportDone(host.jwt); + + // Put the ZIP back into the state it occupies for most of a real release: still building. + // Everything else in this test is the real product. + await pgQuery( + `UPDATE export_job ej SET status = 'running', progress_pct = 50 + FROM event e WHERE e.id = ej.event_id AND e.slug = '${SLUG}' AND ej.type = 'zip'` + ); + + // A guest deletes their own comment → Affects::ViewerOnly. + const del = await fetch(BASE + `/api/v1/comment/${commentId}`, { + method: 'DELETE', + headers: { Authorization: `Bearer ${g.jwt}` }, + }); + expect(del.status).toBe(204); + + // The ZIP must have been RE-ARMED at the live epoch, not abandoned at the retired one. + expect(await zipJobEpoch(), 'the ZIP must not be stranded at a retired epoch').toBe( + await eventEpoch() + ); + + // And it must actually become downloadable again, with its media intact. + await waitExportDone(host.jwt); + expect(await downloadZipEntries(host.jwt)).toHaveLength(1); + }); + test('a release broadcasts export-progress 100 for both types and one export-available', async ({ host, }) => {