fabi/EventSnap
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(export): single epoch authority; fix upload-path TOCTOU losing photos

Browse Source 
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>
This commit is contained in:
fabi
2026-07-14 19:48:38 +02:00
parent 1148c2e906
commit 8e906d7866
12 changed files with 802 additions and 430 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
e2e/specs/06-export/export.spec.ts
View File

@@ -54,27 +54,32 @@ test.describe('Export — release and download', () => {
return (await res.json()).ticket;
}
test('ZIP download 404s when the export is not yet marked ready', async ({ guest, db }) => {
test('ZIP download 404s for a `done` job at a RETIRED epoch', async ({ guest, db }) => {
const g = await guest('NotReady');
// Released flag set, but export_zip_ready is still false → must refuse, never serve.
// The event is released and the zip job says `done` — but the job carries a dead epoch, which
// is exactly the state a reopen (or a superseded worker) leaves behind. Readiness is derived
// from `job.epoch = event.export_epoch`, so this archive is NOT current and must never be
// served. A 200 here would be the stale-keepsake bug leaking through the read path.
await db.setExportReleased(SLUG, true);
await db.fakeExportJob(SLUG, 'zip', 'done');
await db.setExportZipReady(SLUG, false); // retire the job to a dead epoch
const ticket = await mintTicket(g.jwt);
const res = await fetch(base + '/api/v1/export/zip?ticket=' + encodeURIComponent(ticket));
// Pinned to 404 (not [404,200]): a 200 here would mean serving an export that was
// never released for download — a data-exposure regression. This hits the
// `!export_zip_ready` guard.
expect(res.status).toBe(404);
});
test('ZIP download 404s when marked ready but the file is missing on disk', async ({ guest, db }) => {
test('ZIP download 404s when the job is current but the file is missing on disk', async ({
guest,
db,
}) => {
const g = await guest('ReadyNoFile');
// Released AND ready, but no Gallery.zip on disk (we never ran a real export) →
// the handler must 404 on the missing-file check, not 200/500 or serve a stale file.
// Released, and the job is `done` at the LIVE epoch — but no archive was ever written (we
// never ran a real export). The handler must 404 on the missing file, not 200/500 or serve
// something stale.
await db.setExportReleased(SLUG, true);
await db.setExportZipReady(SLUG, true);
await db.fakeExportJob(SLUG, 'zip', 'done');
await db.setExportZipReady(SLUG, true);
const ticket = await mintTicket(g.jwt);
const res = await fetch(base + '/api/v1/export/zip?ticket=' + encodeURIComponent(ticket));

214
e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts
View File

@@ -11,20 +11,16 @@
* `export_*_ready = TRUE` on an archive that predated the reopen — so uploads added
* during the reopen window were permanently missing from a "ready" keepsake.
*
* The fix: a per-(event,type) generation counter `release_seq`, bumped on every (re)release.
* A worker captures the seq it claimed and writes per-generation temp/final paths
* (`Gallery.<seq>.zip`); its finalize and ready-flag flip are guarded by `release_seq`, so a
* superseded worker discards its output instead of resurrecting a stale keepsake. The
* download follows the current `done` row's `file_path`, never a fixed name.
* The fix (migration 014): ONE monotonic `event.export_epoch`, bumped in the SAME UPDATE as any
* change to `export_released_at` — release and reopen are its only writers. Each export_job row
* carries a COPY of the epoch it was enqueued for. An export is downloadable IFF
* `released AND job.epoch = event.export_epoch AND job.status = 'done'` — readiness is DERIVED,
* never stored, so it cannot drift and no worker can resurrect it.
*
* 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 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.
* The consequence that kills the whole bug class: a worker holding a retired epoch is INERT BY
* CONSTRUCTION. Every write it makes is guarded on `epoch` on the very row it updates, so it simply
* matches nothing — no cross-table EXISTS (which would be unsound under READ COMMITTED), no flag to
* flip, nothing to skip. Losing a race can no longer corrupt state; it can only waste work.
*
* Coverage:
* - churn integrity: rapid reopen→re-release yields exactly one INTACT ZIP, no stuck jobs.
@@ -32,9 +28,11 @@
* keepsake (the direct data-loss regression).
* - 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).
* - supersession (reopen): a reopen ALONE bumps the event epoch, so a worker holding the
* pre-reopen epoch is retired and can publish nothing.
* - acceptance: every upload the server ACCEPTED (201) is in the keepsake — the upload-path
* TOCTOU that was the real cause of the "stale keepsake" bug.
* - takedown: deleting a photo AFTER release regenerates the keepsake without it.
*/
import { test, expect } from '../../fixtures/test';
import { Client } from 'pg';
@@ -49,6 +47,8 @@ import { SseListener } from '../../helpers/sse-listener';
const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101';
const SLUG = 'e2e-test-event';
const N_UPLOADS = 6;
/** A ZIP holding no entries: just the 22-byte end-of-central-directory record. */
const EMPTY_ZIP_BYTES = 22;
const PG = {
host: process.env.E2E_DB_HOST ?? 'localhost',
@@ -98,22 +98,40 @@ async function downloadZipEntries(jwt: string): Promise<string[]> {
expect(res.status).toBe(200);
const bytes = Buffer.from(await res.arrayBuffer());
expect(bytes.subarray(0, 2).toString('latin1')).toBe('PK'); // ZIP magic — not a stomped file
// A ZIP with no entries is exactly its 22-byte end-of-central-directory record. That is
// structurally VALID (an event with nothing to export), but `unzip -t` exits non-zero on it — so
// check for it explicitly rather than letting it masquerade as corruption.
if (bytes.length === EMPTY_ZIP_BYTES) return [];
const dir = mkdtempSync(join(tmpdir(), 'es-zip-'));
const zipPath = join(dir, 'Gallery.zip');
writeFileSync(zipPath, bytes);
execFileSync('unzip', ['-t', zipPath], { stdio: 'pipe' }); // CRC integrity; throws on corruption
try {
execFileSync('unzip', ['-t', zipPath], { stdio: 'pipe' }); // CRC integrity; throws on corruption
} catch (e: any) {
const out = `${e.stdout?.toString() ?? ''}${e.stderr?.toString() ?? ''}`;
throw new Error(`keepsake ZIP failed integrity check (${bytes.length} bytes):\n${out}`);
}
return execFileSync('unzip', ['-Z1', zipPath], { encoding: 'utf8' })
.split('\n')
.map((l) => l.trim())
.filter((l) => l.length > 0 && !l.endsWith('/'));
}
async function zipReleaseSeq(): Promise<number> {
const [row] = await pgQuery<{ release_seq: string }>(
`SELECT ej.release_seq FROM export_job ej JOIN event e ON e.id = ej.event_id
/** The epoch the CURRENT zip job row was enqueued for. */
async function zipJobEpoch(): Promise<number> {
const [row] = await pgQuery<{ epoch: string }>(
`SELECT ej.epoch FROM export_job ej JOIN event e ON e.id = ej.event_id
WHERE e.slug = '${SLUG}' AND ej.type = 'zip'`
);
return Number(row.release_seq);
return Number(row.epoch);
}
/** The event's authoritative generation counter. */
async function eventEpoch(): Promise<number> {
const [row] = await pgQuery<{ export_epoch: string }>(
`SELECT export_epoch FROM event WHERE slug = '${SLUG}'`
);
return Number(row.export_epoch);
}
test.describe('Flow re-review — reopen→re-release export integrity + stale-keepsake (H1)', () => {
@@ -138,9 +156,9 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k
});
expect(rejected.status).toBe(403);
// Churn: reopen (clears release + ready flags) then re-release, several times in
// quick succession. This is the path that used to be able to double-spawn workers
// over the shared temp file. End on a release so the gallery is left released.
// Churn: reopen (retires the generation) then re-release, several times in quick
// succession. This is the path that used to double-spawn workers over a shared temp file.
// End on a release so the gallery is left released.
for (let i = 0; i < 4; i++) {
expect((await post('/api/v1/host/event/open', host.jwt)).status).toBe(204);
const rel = await post('/api/v1/host/gallery/release', host.jwt);
@@ -194,14 +212,14 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k
//
// OLD behavior (the bug): the re-enqueue left a `running` row alone, so the stale worker
// would eventually finalize and re-flip the ready flag on its pre-reopen snapshot.
// NEW behavior (the fix): the re-enqueue bumps `release_seq` and resets the row, so the
// NEW behavior (the fix): the re-enqueue bumps `epoch` and resets the row, so the
// stale generation is superseded — its sentinel is wiped and a fresh worker regenerates.
await seedUpload(host.jwt, { caption: 'a' });
await seedUpload(host.jwt, { caption: 'b' });
expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204);
await waitExportDone(host.jwt);
const seqBefore = await zipReleaseSeq();
const seqBefore = await zipJobEpoch();
// Simulate an in-flight worker owning the zip job at the current generation.
await pgQuery(
@@ -210,61 +228,65 @@ 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 AND bumps every export_job's release_seq) then
// Reopen (clears release + ready flags AND bumps every export_job's epoch) 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);
// The fresh generation must settle to done at a HIGHER release_seq — proving the stale
// The fresh generation must settle to done at a HIGHER epoch — proving the stale
// running row was superseded (not left to finalize) and the sentinel is gone.
await waitExportDone(host.jwt);
const [row] = await pgQuery<{ status: string; progress_pct: number; release_seq: string }>(
`SELECT ej.status::text AS status, ej.progress_pct, ej.release_seq
const [row] = await pgQuery<{ status: string; progress_pct: number; epoch: string }>(
`SELECT ej.status::text AS status, ej.progress_pct, ej.epoch
FROM export_job ej JOIN event e ON e.id = ej.event_id
WHERE e.slug = '${SLUG}' AND ej.type = 'zip'`
);
expect(row.status, 'zip job status').toBe('done');
expect(Number(row.progress_pct), 'zip job progress').toBe(100);
expect(Number(row.release_seq), 'release_seq advanced past the superseded generation').toBeGreaterThan(
expect(Number(row.epoch), 'epoch advanced past the superseded generation').toBeGreaterThan(
seqBefore
);
});
test('a reopen ALONE bumps release_seq — a pre-reopen worker can never flip a stale ready flag', async ({
test('a reopen ALONE bumps the epoch — a pre-reopen worker is retired and can publish nothing', 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.
// Deterministic proof that a reopen retires the current generation. The nasty interleavings
// (a worker finalizing around a reopen/re-release) can't be forced by sub-millisecond timing —
// but their ROOT cause is observable: whether a reopen moves the epoch. If it does, the epoch
// the worker captured is gone for good, so its epoch-guarded finalize matches nothing no matter
// when it lands, and there is no separate ready flag left for it to resurrect.
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();
const epochAtRelease = await eventEpoch();
expect(await zipJobEpoch(), 'the done job carries the live epoch').toBe(epochAtRelease);
// 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.
// Reopen only — no re-release.
expect((await post('/api/v1/host/event/open', host.jwt)).status).toBe(204);
const seqAfterReopen = await zipReleaseSeq();
const epochAfterReopen = await eventEpoch();
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);
epochAfterReopen,
`reopen must move the event epoch past ${epochAtRelease}, retiring any worker holding it`
).toBeGreaterThan(epochAtRelease);
// 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}'`
// The job row still carries the OLD epoch — so it no longer matches the event's, which is
// exactly what makes it (and any worker holding it) invisible and inert.
expect(await zipJobEpoch(), 'the stale job row is left behind at the retired epoch').toBe(
epochAtRelease
);
// Readiness is DERIVED, so there is no flag to check — the keepsake simply stops being
// downloadable the moment the epoch moves. Assert the observable contract instead.
const [ev] = await pgQuery<{ released_at: string | null }>(
`SELECT 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();
const ticket = await mintTicket(host.jwt);
const dl = await fetch(BASE + '/api/v1/export/zip?ticket=' + encodeURIComponent(ticket));
expect(dl.status, 'a reopened event serves no keepsake').toBe(404);
});
test('open ‖ release churn always converges to a consistent, downloadable keepsake', async ({
@@ -273,9 +295,9 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k
// Convergence STRESS test, not a regression guard — be honest about what this does and does
// not prove. `open_event` reopens the event AND bumps every export_job generation, and those
// two statements must be atomic (they run in one transaction): if they were separate
// autocommits, a re-release could slip between them, spawn a FRESH worker at seq N+1, and the
// reopen's second statement would bump that live generation to N+2 — orphaning the worker,
// stranding its row at `running` forever with the host unable to retry.
// autocommits, a re-release could slip between them and spawn a FRESH worker whose generation the
// reopen's second statement would then retire — orphaning it and stranding its row at `running`
// forever with the host unable to retry. open_event is now a single UPDATE, so this cannot occur.
//
// That interleaving is NOT reproducible from outside the process (verified: this test passes
// against a deliberately non-transactional build even with ~90 concurrent open/release pairs —
@@ -320,6 +342,84 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k
expect((await downloadZipEntries(host.jwt)).length).toBe(1);
});
test('EVERY upload the server accepted is in the keepsake (upload-path release TOCTOU)', async ({
host,
}) => {
// THE contract, and the bug that survived three rounds of fixes to the export state machine
// because it was never in the state machine:
//
// `upload` checked `uploads_locked_at` BEFORE streaming the body (minutes, for a big video)
// and then committed the row WITHOUT re-checking. So 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. Nothing ever regenerated it.
//
// The invariant is absolute and does not depend on winning any race: if the server said 201,
// that photo is in the keepsake. If it said 403, it isn't. Race uploads against a release and
// hold the server to exactly that.
const bytes = readFileSync(join(process.cwd(), 'fixtures', 'media', 'sample.jpg'));
// One upload that is definitely committed before the race, so the keepsake is never trivially
// empty (the release can otherwise win every race and the assertion would prove nothing).
await seedUpload(host.jwt, { caption: 'pre-race' });
const inflight = Array.from({ length: 10 }, (_, i) =>
uploadRaw(host.jwt, bytes, { filename: `race${i}.jpg`, contentType: 'image/jpeg' })
);
// Fire the release while those are in flight.
const releasing = post('/api/v1/host/gallery/release', host.jwt);
const results = await Promise.all(inflight);
await releasing;
const accepted = results.filter((r) => r.status >= 200 && r.status < 300).length;
const rejected = results.filter((r) => r.status === 403).length;
expect(accepted + rejected, 'every upload either succeeded or was locked out').toBe(
results.length
);
await waitExportDone(host.jwt);
const listing = await downloadZipEntries(host.jwt);
// +1 for the pre-race upload.
expect(
listing.length,
`server accepted ${accepted} racing upload(s) (+1 pre-race) and locked out ${rejected}, but ` +
`the keepsake holds ${listing.length}. An accepted photo missing from the keepsake is ` +
`silent, permanent data loss — exactly the bug this guards.`
).toBe(accepted + 1);
});
test('deleting a photo AFTER release regenerates the keepsake without it (takedown)', async ({
host,
}) => {
// A host deleting a photo on a removal request used to remove it from the feed but leave it in
// the already-generated archive FOREVER — the `if ready { continue }` skip guaranteed the
// export was never rebuilt. The keepsake is the one place it most needs to be gone.
const keep = await seedUpload(host.jwt, { caption: 'keep' });
const takedown = await seedUpload(host.jwt, { caption: 'takedown' });
expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204);
await waitExportDone(host.jwt);
expect((await downloadZipEntries(host.jwt)).length).toBe(2);
// Take it down while the gallery is released.
const del = await fetch(BASE + `/api/v1/host/upload/${takedown}`, {
method: 'DELETE',
headers: { Authorization: `Bearer ${host.jwt}` },
});
expect(del.status).toBe(204);
// The keepsake must rebuild — and must NOT still contain the removed photo.
await waitExportDone(host.jwt);
const listing = await downloadZipEntries(host.jwt);
expect(listing.length, `zip entries:\n${listing.join('\n')}`).toBe(1);
expect(
listing.some((n) => n.includes(takedown)),
'the taken-down photo must be gone from the regenerated keepsake'
).toBe(false);
expect(listing.some((n) => n.includes(keep)), 'the kept photo survives').toBe(true);
});
test('a release broadcasts export-progress 100 for both types and one export-available', async ({
host,
}) => {