8e906d7866
A deep investigative review of the export concurrency (four parallel adversarial
reviewers) found that three rounds of race fixes had been chasing the wrong bug, and
that the model itself was the root cause. This replaces the model and fixes the real
data loss.
THE ACTUAL BUG (upload path, not the state machine)
`upload` checked `uploads_locked_at` BEFORE streaming the body — minutes, for a large
video — then committed the row without re-checking. A release landing mid-upload let the
row commit AFTER the export snapshot: the photo appeared in the live feed and was
PERMANENTLY missing from the keepsake, with nothing to regenerate it. release_gallery's
comment claims to prevent exactly this; it never did. Every prior round fixed the DB
state machine, and the leak was upstream of it.
Fix: re-assert the lock under `SELECT ... FOR SHARE` INSIDE the commit tx. That row lock
conflicts with release_gallery's `UPDATE event`, so either the upload commits first (and
the release — hence the snapshot — is ordered after it) or the release wins and the
upload is rejected as `uploads_locked` (reversible; the client keeps the blob).
Regression test verified NON-VACUOUS: with the fix reverted it fails, reporting accepted
uploads missing from the keepsake.
THE MODEL (migration 014)
"Which generation is current?" had no single answer: it was assembled from three
separately-written pieces across two tables (`export_released_at`, a per-job
`release_seq`, and cached `export_{zip,html}_ready` flags). Keeping them in agreement
took ~10 hand-written guards; each review round found one more path that slipped through.
Now: ONE monotonic `event.export_epoch`, bumped in the same UPDATE as any change to
`export_released_at`. Each job carries a copy. Downloadable IFF
`released AND job.epoch = event.export_epoch AND status = 'done'` — readiness DERIVED,
never stored. A retired-epoch worker is INERT BY CONSTRUCTION: losing a race can no
longer corrupt state, only waste work.
Deleted: both ready-flag columns, both ready-flip blocks, `if ready { continue }`, the
reopen seq-bump, open_event's transaction, and the cross-table claim guard.
That last one was also UNSOUND: under READ COMMITTED, a blocked UPDATE re-evaluates its
WHERE against the updated target row but answers subqueries on OTHER tables from the
original snapshot — so the previous `EXISTS (SELECT ... FROM event ...)` claim guard
could admit a claim against an already-reopened event while RETURNING the post-bump seq.
Every guard is now same-row (`epoch = $n`), which EPQ re-evaluates correctly.
OTHER REAL DEFECTS FIXED
- release_gallery committed the release and THEN awaited the enqueue inside the handler
future. Axum drops that future on client disconnect → event released, uploads locked,
ZERO export_job rows, host unable to retry ("bereits freigegeben"), restart-only
recovery. Now one tx; workers spawn (detached) after commit.
- No flush/fsync before the tmp→final rename. async_zip's close() never flushes the inner
file and tokio's File drop DISCARDS write errors — so a full disk silently produced a
truncated archive that was then marked done and published, after which the last good
generation was pruned. Now flush + sync_all, which also surfaces ENOSPC.
- Download read the ready flag and the file path in TWO queries; a reopen between them
served a keepsake that should 404. Now one read through the `export_current` view.
- `if !src.exists() { continue }` then `open()?` — a TOCTOU that turned a tolerated gap
into a hard failure of the ENTIRE keepsake (unretryable while released). Now open-first,
warn, and skip.
- Recovery was flag-driven and never checked the disk: a `done` row whose archive was gone
was a permanent dead end needing manual DB surgery. Now verifies the file and re-arms.
- Temp files/dirs leaked on every error path (the leak is what fills the disk that
corrupts the next archive). Now cleaned up.
- Export archives were not event-scoped (`viewer_tmp_` already was), so a second event
would collide on paths and one event's prune would delete another's live keepsake.
- Host deletes after release never regenerated the keepsake — a photo removed on request
lived on in the archive forever. Now bumps the epoch and rebuilds without it.
- claim_job swallowed DB errors as "someone else won", stranding a job pending at 0%.
- export_status reported retired-epoch rows (a progress bar that never moves).
- The startup sweep's single-instance assumption is now documented, not hidden.
Verified: backend 40 tests, svelte-check 0 errors, e2e typecheck clean,
e2e 160 passed / 1 skipped on chromium-desktop (incl. 2 new regressions; the
upload-acceptance one confirmed to fail without the fix).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
29 lines
1.1 KiB
SQL
29 lines
1.1 KiB
SQL
-- Restore the stored ready flags and the per-job counter.
|
|
DROP VIEW IF EXISTS export_current;
|
|
|
|
ALTER TABLE event ADD COLUMN export_zip_ready BOOLEAN NOT NULL DEFAULT FALSE;
|
|
ALTER TABLE event ADD COLUMN export_html_ready BOOLEAN NOT NULL DEFAULT FALSE;
|
|
|
|
-- Re-derive the flags from what the epoch model considers downloadable, so the old code sees
|
|
-- exactly the state it would have written itself.
|
|
UPDATE event e
|
|
SET export_zip_ready = EXISTS (
|
|
SELECT 1 FROM export_job j
|
|
WHERE j.event_id = e.id AND j.type = 'zip'
|
|
AND j.status = 'done' AND j.epoch = e.export_epoch
|
|
),
|
|
export_html_ready = EXISTS (
|
|
SELECT 1 FROM export_job j
|
|
WHERE j.event_id = e.id AND j.type = 'html'
|
|
AND j.status = 'done' AND j.epoch = e.export_epoch
|
|
)
|
|
WHERE e.export_released_at IS NOT NULL;
|
|
|
|
ALTER TABLE export_job RENAME COLUMN epoch TO release_seq;
|
|
|
|
-- The old code treats release_seq as a non-negative per-job counter; the -1 retirement sentinel
|
|
-- has no meaning there, so floor it back to 0.
|
|
UPDATE export_job SET release_seq = 0 WHERE release_seq < 0;
|
|
|
|
ALTER TABLE event DROP COLUMN export_epoch;
|