A 6-lens multi-agent review (103 agents; every finding adversarially verified by three
refute-by-default skeptics) confirmed the epoch redesign is sound — the core invariant
holds — and found 13 real defects around it. All are fixed here.
The redesign's invariant was re-verified and stands: an accepted upload is always in the
keepsake, and a downloadable keepsake always reflects the most recent release.
But a WEAKER adjacent invariant did NOT hold — "a downloadable keepsake reflects the
current content" — and that is the theme of the biggest fixes.
CONTENT REMOVAL NOW ALWAYS REACHES THE KEEPSAKE
Regeneration was wired only to host deletes. Ban, unban, guest self-delete and guest
comment-delete did not trigger it — yet the export query filters `is_banned = FALSE`,
so the pipeline already agreed that content must not be there. Ban someone for abusive
content after release and every guest kept downloading an archive containing it.
All five removal paths now invalidate and rebuild. `query_comments` also gained the
banned/hidden filter it was missing entirely (the ZIP had it; the viewer did not).
Each removal is now ATOMIC with its invalidation (one tx, models made executor-generic).
Previously the delete committed in its own tx and the regeneration in a second: a dropped
handler future between them left the taken-down photo permanently downloadable, and
nothing could notice — the keepsake still looked complete and the host could no longer
even find the upload to retry.
NO MORE TAKEDOWN STORM
Every removal spawned two uncancellable full exports. A superseded worker was INERT, not
STOPPED: it still ran every ffmpeg spawn and image resize and wrote a whole archive before
discovering it had lost. Five deletes left ten workers alive, each holding a
full-gallery-sized temp, in a 1 GB container.
Now: a debounce before `claim_job` (a superseded worker fails its claim and does ZERO
work, collapsing a burst into one export), plus `update_progress` — which already ran
exactly the right liveness predicate and threw the answer away — returning it so both file
loops bail the instant they are retired. Moderating a comment no longer rebuilds the
multi-GB ZIP either: the ZIP is media-only, so it is carried forward, with prune taught to
protect any file a live row still references.
AN ESCAPE HATCH
A failed export was terminal at runtime: release_gallery refuses an already-released event,
recovery only runs at boot, and there was no retry route — the only outs were restarting the
container or reopening uploads to every guest. Added POST /host/export/rebuild. Also widened
mark_failed's guard to `status IN ('running','pending')`: when claim_job itself ERRORED, the
row was still `pending`, so the failure was never recorded and the job sat at 0% forever.
SILENT INVALIDATION
Retiring the epoch 404s the download instantly, but nothing told the clients — guests kept
seeing an enabled download button through the whole rebuild. Now broadcasts an invalidation.
And the download was a top-level <a href>: a 404 (or a 429 from the per-IP export limit that
everyone on the venue WiFi shares) NAVIGATED THE USER OUT OF THE PWA onto raw JSON. It now
targets a hidden iframe, so an error response is harmless.
ALSO
- The HTML export still had the exists()-then-open TOCTOU the ZIP path was fixed to remove,
at three sites, two with a hard `?` that failed the ENTIRE viewer. It is reachable: the
compression worker hard-deletes an original when a transcode fails and can still be running
when the gallery is released.
- Crash-orphaned .tmp files were immortal (prune deliberately skips .tmp). Swept at boot,
where it is unambiguously safe.
- export_status read the epoch in one statement and used it in the next; now one query.
- DiskCache::snapshot IGNORED its path argument on a cache hit, returning whatever filesystem
was measured last. Latent only because both callers pass media_path — it would have silently
misreported the moment anyone measured the exports volume. Now keyed by path.
- Corrected two comments that asserted safety properties the code does not have (claim_job
does NOT prevent a retired-epoch claim — retirement is enforced at READ time; and a
multi-replica reap is NOT contained, it can corrupt the keepsake). Added a rollback runbook
to migration 014, the repo's first destructive migration.
TESTS
The regression guard for the headline FOR SHARE fix was probabilistic — it could pass on
broken code if the interleaving fell the wrong way. It is now STRUCTURAL: the upload is held
open mid-body with its pre-flight check already passed, so the release provably lands inside
the exact window. Verified 5/5 FAILING (201 instead of 403) with the fix reverted, 5/5
passing with it.
Verified: backend 40 tests, svelte-check 0 errors, e2e typecheck clean,
e2e 160 passed / 1 skipped on chromium-desktop.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
87 lines
4.6 KiB
SQL
87 lines
4.6 KiB
SQL
-- Export generation, unified.
|
|
--
|
|
-- ⚠ ROLLBACK RUNBOOK. This is the repo's first DESTRUCTIVE migration (it DROPs two columns). The
|
|
-- previous image will NOT boot against this schema: sqlx runs migrations before serving and errors
|
|
-- with VersionMissing on an unknown version, so you get a crash loop, not a degraded service.
|
|
-- To roll back to the previous release you must run 014_export_epoch.down.sql BY HAND FIRST, then
|
|
-- deploy the old image. The down migration re-derives the old ready flags from the epoch state, so
|
|
-- no keepsake is lost.
|
|
--
|
|
-- Before this migration, "which generation of the keepsake is current?" had no single answer.
|
|
-- It was assembled at runtime from three separately-written pieces of state across two tables:
|
|
-- * event.export_released_at — a release marker with NO identity (you cannot ask *which* release)
|
|
-- * export_job.release_seq — a per-row counter, in a different table, bumped in a different statement
|
|
-- * event.export_{zip,html}_ready — a CACHED COPY of the derivation of the other two
|
|
-- Keeping those three in agreement took ~10 hand-written guards, and every guard was a repair of the
|
|
-- same missing invariant. Three consecutive review rounds each found another path that slipped through.
|
|
--
|
|
-- This replaces all of it with ONE authority:
|
|
--
|
|
-- event.export_epoch — a monotonic counter bumped in the SAME UPDATE as any change to
|
|
-- export_released_at (release and reopen are its only writers).
|
|
-- export_job.epoch — a COPY of event.export_epoch taken when the job was enqueued.
|
|
-- NEVER independently incremented.
|
|
--
|
|
-- The single invariant, which replaces the entire proof:
|
|
--
|
|
-- An export is downloadable IFF
|
|
-- event.export_released_at IS NOT NULL
|
|
-- AND export_job.epoch = event.export_epoch
|
|
-- AND export_job.status = 'done'
|
|
--
|
|
-- Readiness is therefore DERIVED, never stored — so it cannot drift, and no worker can set it.
|
|
-- A worker holding a dead epoch is INERT BY CONSTRUCTION: anything it writes is invisible to every
|
|
-- reader, with no guard involved. Losing a race can no longer corrupt state; it can only waste work.
|
|
--
|
|
-- Crucially, epoch equality is checked on the SAME ROW the worker updates (export_job), never via a
|
|
-- cross-table EXISTS. That matters: under READ COMMITTED, when an UPDATE blocks on a row lock and the
|
|
-- blocker commits, Postgres re-evaluates the WHERE against the *updated target row* but answers
|
|
-- subqueries on OTHER tables from the statement's ORIGINAL snapshot. The old cross-row guard
|
|
-- (`EXISTS (SELECT 1 FROM event WHERE ... export_released_at IS NOT NULL)`) was unsound for exactly
|
|
-- that reason — it could admit a claim against an already-reopened event while RETURNING the
|
|
-- post-bump seq. A same-row `epoch = $n` predicate is re-evaluated correctly by EPQ.
|
|
|
|
ALTER TABLE event ADD COLUMN export_epoch BIGINT NOT NULL DEFAULT 0;
|
|
|
|
-- A currently-released event's live generation becomes epoch 1.
|
|
UPDATE event SET export_epoch = 1 WHERE export_released_at IS NOT NULL;
|
|
|
|
-- The per-job counter becomes a plain copy of the event epoch it was enqueued for.
|
|
ALTER TABLE export_job RENAME COLUMN release_seq TO epoch;
|
|
|
|
-- Carry the OLD notion of "downloadable" across exactly: a job the old model considered ready
|
|
-- (released + done + its ready flag) adopts the event's live epoch. Everything else is retired to
|
|
-- -1, which can never equal a non-negative event.export_epoch, so it is inert forever.
|
|
UPDATE export_job j
|
|
SET epoch = CASE
|
|
WHEN e.export_released_at IS NOT NULL
|
|
AND j.status = 'done'
|
|
AND ((j.type = 'zip' AND e.export_zip_ready)
|
|
OR (j.type = 'html' AND e.export_html_ready))
|
|
THEN e.export_epoch
|
|
ELSE -1
|
|
END
|
|
FROM event e
|
|
WHERE e.id = j.event_id;
|
|
|
|
-- The duplicated derivation. Gone — along with the whole class of bugs where a stale worker
|
|
-- resurrected it, or where it disagreed with the job row it was supposed to summarise.
|
|
ALTER TABLE event DROP COLUMN export_zip_ready;
|
|
ALTER TABLE event DROP COLUMN export_html_ready;
|
|
|
|
-- The ONE definition of "the current generation". Every reader goes through this, so readiness and
|
|
-- the file path are resolved in a SINGLE read — closing the download-path TOCTOU where the ready
|
|
-- flag was checked in one query and file_path fetched in another (a reopen between the two served a
|
|
-- keepsake that should have 404'd).
|
|
CREATE VIEW export_current AS
|
|
SELECT j.event_id,
|
|
j.type,
|
|
j.status,
|
|
j.progress_pct,
|
|
j.file_path,
|
|
j.epoch
|
|
FROM export_job j
|
|
JOIN event e ON e.id = j.event_id
|
|
WHERE e.export_released_at IS NOT NULL -- implied by the epoch match; kept as an assertion
|
|
AND j.epoch = e.export_epoch;
|