fix(rereview): close residual export finalize↔flip double-race at reopen
Adversarial re-review of df275bb found the two-guard ready-flip fix still left a
narrow double-race open, plus test-hygiene drift from the banUser param removal.
MED — residual stale-keepsake race the `export_released_at` guard alone missed
The flip relied on two guards defeating two clearers: the `release_seq` EXISTS
check (vs a re-release bump) and `export_released_at IS NOT NULL` (vs open_event's
clear). But release_gallery re-arms `export_released_at = NOW()` and bumps
`release_seq` as SEPARATE statements, so a stale worker's flip landing in the gap
between them satisfies BOTH guards (released re-armed, seq not yet bumped) and
resurrects a pre-reopen keepsake — the next re-release then skips regeneration and
serves an archive missing the reopen-window uploads.
Root-cause fix: `open_event` now bumps every `export_job.release_seq`, making a
reopen a supersession point symmetric with a re-release. A worker that captured the
pre-reopen seq can never match its `release_seq`-guarded finalize/flip again,
regardless of when the re-release re-arms `export_released_at`. The released-anchor
guard stays as defense-in-depth. Added a deterministic e2e asserting the reopen
bumps the seq (the invariant that closes the race without depending on
sub-millisecond worker timing).
LOW
- Gate the `export-progress: 100` SSE + `prune_stale_export_files` on the ready-flip
actually flipping (rows_affected > 0). A superseded worker no longer advertises a
misleading 100% or prunes on a fresh generation's behalf.
- moderation.spec.ts: the two ban tests had become byte-identical after the
hide_uploads param removal; drop the one whose "hide_uploads=true" title no longer
matched what it exercised, keep the accurate "always hides" test.
- host-dashboard page-object: drop the dead `banUser(hideUploads)` param + its
checkbox branch (the UI no longer renders that checkbox — a latent hang trap).
- sse-eviction.spec.ts: rename the test whose title referenced the removed flag.
Verified: backend 40 tests, e2e 156 passed / 1 skipped on chromium-desktop
(incl. the new reopen-supersession regression).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -7,18 +7,9 @@ import { test, expect } from '../../fixtures/test';
|
||||
import { seedUpload, seedComment } from '../../helpers/seed';
|
||||
|
||||
test.describe('Host — moderation API', () => {
|
||||
test('ban with hide_uploads=true sets the right flags', async ({ api, host, guest }) => {
|
||||
const target = await guest('Banned1');
|
||||
await api.banUser(host.jwt, target.userId);
|
||||
const users = await api.listUsers(host.jwt);
|
||||
const row = users.find((u: any) => u.id === target.userId);
|
||||
expect(row?.is_banned).toBe(true);
|
||||
expect(row?.uploads_hidden).toBe(true);
|
||||
});
|
||||
|
||||
test('ban always hides — the hide_uploads flag is ignored (USER_JOURNEYS §10.5)', async ({ api, host, guest }) => {
|
||||
// Ban is now unconditionally a hide: even asking NOT to hide still hides, because a
|
||||
// banned user's content is "gone" everywhere. The legacy hide_uploads arg is ignored.
|
||||
test('ban always hides — a banned user is is_banned AND uploads_hidden (USER_JOURNEYS §9.5)', async ({ api, host, guest }) => {
|
||||
// Ban is unconditionally a hide: a banned user's content is "gone" everywhere. The endpoint
|
||||
// takes no body and there is no per-request opt-out (the legacy hide_uploads flag is gone).
|
||||
const target = await guest('Banned2');
|
||||
await api.banUser(host.jwt, target.userId);
|
||||
const users = await api.listUsers(host.jwt);
|
||||
|
||||
@@ -30,7 +30,7 @@ test.describe('Host — live SSE eviction (H3)', () => {
|
||||
);
|
||||
});
|
||||
|
||||
test('banning a user with hide_uploads broadcasts user-hidden', async ({
|
||||
test('banning a user broadcasts user-hidden', async ({
|
||||
api,
|
||||
host,
|
||||
guest,
|
||||
|
||||
@@ -18,23 +18,23 @@
|
||||
* download follows the current `done` row's `file_path`, never a fixed name.
|
||||
*
|
||||
* 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. `open_event`
|
||||
* clears `export_released_at` + both ready flags but leaves the `export_job` row `done` at the
|
||||
* current seq — so the seq-guarded flip would still match and resurrect `export_zip_ready=TRUE`
|
||||
* on a pre-reopen snapshot, and the next re-release would `if ready { continue }` and skip
|
||||
* regeneration. The flip UPDATEs are therefore additionally anchored on
|
||||
* `export_released_at IS NOT NULL`, making them a no-op once a reopen has landed. The
|
||||
* "completeness" test below exercises the real reopen→re-release worker path end-to-end; the
|
||||
* sub-millisecond finalize↔flip interleave itself isn't deterministically forceable with the
|
||||
* fast fixtures, so that exact window is covered by the SQL guard + code review rather than a
|
||||
* timing-dependent assertion.
|
||||
* 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.
|
||||
*
|
||||
* Coverage:
|
||||
* - churn integrity: rapid reopen→re-release yields exactly one INTACT ZIP, no stuck jobs.
|
||||
* - completeness: an upload added during the reopen window IS present in the re-released
|
||||
* keepsake (the direct data-loss regression).
|
||||
* - supersession: a re-release SUPERSEDES an in-flight (running) job — it bumps the
|
||||
* - 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).
|
||||
*/
|
||||
import { test, expect } from '../../fixtures/test';
|
||||
import { Client } from 'pg';
|
||||
@@ -209,7 +209,8 @@ 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; does NOT touch job rows) then re-release.
|
||||
// Reopen (clears release + ready flags AND bumps every export_job's release_seq) 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);
|
||||
|
||||
@@ -227,4 +228,41 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k
|
||||
seqBefore
|
||||
);
|
||||
});
|
||||
|
||||
test('a reopen ALONE bumps release_seq — a pre-reopen worker can never flip a stale ready flag', 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.
|
||||
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();
|
||||
|
||||
// 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.
|
||||
expect((await post('/api/v1/host/event/open', host.jwt)).status).toBe(204);
|
||||
|
||||
const seqAfterReopen = await zipReleaseSeq();
|
||||
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);
|
||||
|
||||
// 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}'`
|
||||
);
|
||||
expect(ev.zip_ready, 'reopen clears zip readiness').toBe(false);
|
||||
expect(ev.released_at, 'reopen clears the release timestamp').toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user