fix(rereview): close export-flip race + queue-dedup reload regression
Adversarial re-review of the persona-audit + audit-followup rounds (6411747..)
found one HIGH and one MED regression plus LOW gaps. All fixed with coverage.
HIGH — export stale-keepsake resurrected by an open_event race
A reopen landing in the window between a *current* export worker's finalize_job
and its ready-flag flip cleared export_released_at + the ready flags but left the
export_job row `done` at the same release_seq. The seq-guarded flip then still
matched and re-set export_{zip,html}_ready=TRUE on a pre-reopen snapshot; the next
re-release read that stale TRUE and skipped regeneration (`if ready { continue }`),
serving a keepsake missing every upload from the reopen window — the exact data
loss migration 012 exists to prevent. Both ready-flip UPDATEs are now additionally
anchored on `export_released_at IS NOT NULL`, so a landed reopen makes the flip a
no-op and the re-release regenerates cleanly.
MED — queue dedup broke for reloaded items
loadQueue rebuilt QueueItems from IndexedDB without copying lastModified, which the
new addToQueue dedup keys on. A file re-selected after a page reload / PWA relaunch
missed the duplicate check and uploaded twice. Rehydration now carries lastModified
(extracted to a pure, tested entryToQueueItem helper).
LOW
- diashow: clear the upload-processed debounce timer in onDestroy (no stray
post-unmount /feed fetch).
- USER_JOURNEYS §9.5: document the reconnect-delta ban replay (hidden_user_ids /
uploads_hidden_at, migration 013), not just the live user-hidden SSE.
- e2e api-client: drop the misleading hide_uploads param from banUser — the backend
takes no body and always hides; strip the dead boolean at all call sites.
Tests
- Extract isReversibleLock (the terminal-403 KEEP-vs-PURGE-blob discriminator) into a
pure exported helper + unit tests, so the data-loss-critical branch is covered
without an XHR harness.
- entryToQueueItem unit tests lock the lastModified-carry regression.
- Document the export flip-race guard in the reopen/re-release spec (the sub-ms
finalize↔flip interleave isn't deterministically forceable with fast fixtures;
covered by the SQL guard + the end-to-end completeness test).
Verified: backend 40 tests, frontend 44 unit tests, svelte-check 0 errors,
e2e 156 passed / 1 skipped on chromium-desktop.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -1,5 +1,5 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { classifyUploadStatus } from './upload-queue';
|
||||
import { classifyUploadStatus, isReversibleLock, entryToQueueItem } from './upload-queue';
|
||||
|
||||
/**
|
||||
* Regression guard for the upload-queue retry policy (H2 + M1). The bug being locked out:
|
||||
@@ -40,3 +40,70 @@ describe('classifyUploadStatus', () => {
|
||||
expect(classifyUploadStatus(503)).toBe('transient');
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Regression guard for the reversible-lock discrimination inside the `terminal` bucket — the
|
||||
* branch that decides whether a 4xx KEEPS the blob (event closed / gallery released: a host can
|
||||
* reopen and the photo resumes) or PURGES it (permanent ban / quota). Getting this wrong either
|
||||
* loses a photo the guest expected to survive a reopen, or lets a banned device retry forever.
|
||||
*/
|
||||
describe('isReversibleLock', () => {
|
||||
it('an `uploads_locked` code is reversible at any status (event closed / released)', () => {
|
||||
expect(isReversibleLock(403, 'uploads_locked')).toBe(true);
|
||||
expect(isReversibleLock(409, 'uploads_locked')).toBe(true);
|
||||
});
|
||||
|
||||
it('a `forbidden` 403 (banned) is PERMANENT — purge, never resume', () => {
|
||||
expect(isReversibleLock(403, 'forbidden')).toBe(false);
|
||||
});
|
||||
|
||||
it('an unidentifiable 403 (unparseable proxy/WAF/captive-portal body) is treated reversible', () => {
|
||||
// Losing a photo is the worst outcome; 403 is the reversible-lock status here.
|
||||
expect(isReversibleLock(403, undefined)).toBe(true);
|
||||
expect(isReversibleLock(403, null)).toBe(true);
|
||||
expect(isReversibleLock(403, '')).toBe(true);
|
||||
});
|
||||
|
||||
it('a non-403 permanent 4xx (e.g. 413 quota) is NOT reversible unless explicitly locked', () => {
|
||||
expect(isReversibleLock(413, undefined)).toBe(false);
|
||||
expect(isReversibleLock(400, 'bad_request')).toBe(false);
|
||||
expect(isReversibleLock(413, 'uploads_locked')).toBe(true); // explicit tag still wins
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Regression guard for the queue-rehydration mapping. The bug this locks out: `loadQueue`
|
||||
* rebuilt items from IndexedDB WITHOUT copying `lastModified`, so a reloaded item had
|
||||
* `lastModified === undefined`. addToQueue's dedup keys on (name, size, lastModified), so
|
||||
* re-selecting the same file after a reload would MISS the duplicate and queue it twice.
|
||||
*/
|
||||
describe('entryToQueueItem', () => {
|
||||
const base = {
|
||||
id: 'e1',
|
||||
userId: 'u1',
|
||||
fileName: 'photo.jpg',
|
||||
fileSize: 1234,
|
||||
lastModified: 1_700_000_000_000,
|
||||
mimeType: 'image/jpeg',
|
||||
status: 'pending' as const
|
||||
};
|
||||
|
||||
it('carries lastModified across rehydration (dedup depends on it)', () => {
|
||||
expect(entryToQueueItem(base).lastModified).toBe(1_700_000_000_000);
|
||||
});
|
||||
|
||||
it('downgrades an interrupted `uploading` entry to `pending` so it resumes', () => {
|
||||
expect(entryToQueueItem({ ...base, status: 'uploading' }).status).toBe('pending');
|
||||
});
|
||||
|
||||
it('a `done` entry reports 100% progress; others start at 0', () => {
|
||||
expect(entryToQueueItem({ ...base, status: 'done' }).progress).toBe(100);
|
||||
expect(entryToQueueItem(base).progress).toBe(0);
|
||||
});
|
||||
|
||||
it('defaults caption/hashtags to empty strings', () => {
|
||||
const item = entryToQueueItem(base);
|
||||
expect(item.caption).toBe('');
|
||||
expect(item.hashtags).toBe('');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -211,6 +211,57 @@ export function classifyUploadStatus(status: number): UploadOutcome {
|
||||
return 'transient';
|
||||
}
|
||||
|
||||
/**
|
||||
* Within the `terminal` bucket, decide whether a 4xx is a REVERSIBLE lock (keep the blob,
|
||||
* park retryable for a host reopen) rather than a permanent rejection (purge the blob).
|
||||
* Pure + exported so this data-loss-critical rule is unit-testable without an XHR harness.
|
||||
*
|
||||
* Reversible when:
|
||||
* - the backend tagged it `uploads_locked` (event closed / gallery released — a host can reopen), OR
|
||||
* - it's ANY 403 we can't positively identify as a permanent ban (`forbidden`). An unparseable
|
||||
* 403 body (proxy/WAF/captive portal) must NOT purge the blob — losing a photo is the worst
|
||||
* outcome, and 403 is the reversible-lock status here.
|
||||
* A `forbidden` 403 (banned) and every non-403 4xx (e.g. 413 quota) are permanent → purge.
|
||||
*/
|
||||
export function isReversibleLock(status: number, errorCode: unknown): boolean {
|
||||
return errorCode === 'uploads_locked' || (status === 403 && errorCode !== 'forbidden');
|
||||
}
|
||||
|
||||
/**
|
||||
* Rehydrate a persisted IndexedDB entry into an in-memory `QueueItem`. Pure + exported so the
|
||||
* field-mapping is unit-testable. The rule that must not regress: `lastModified` MUST be carried
|
||||
* across — addToQueue's dedup keys on it, so an item restored from IndexedDB (page reload / PWA
|
||||
* relaunch) with an undefined lastModified would fail to match a re-selection of the same file
|
||||
* and silently queue it twice. `uploading` is downgraded to `pending` (an interrupted in-flight
|
||||
* upload must resume, not stay stuck spinning).
|
||||
*/
|
||||
export function entryToQueueItem(entry: {
|
||||
id: string;
|
||||
userId: string;
|
||||
fileName: string;
|
||||
fileSize: number;
|
||||
lastModified?: number;
|
||||
mimeType: string;
|
||||
caption?: string;
|
||||
hashtags?: string;
|
||||
status: QueueItem['status'] | 'uploading';
|
||||
error?: string;
|
||||
}): QueueItem {
|
||||
return {
|
||||
id: entry.id,
|
||||
userId: entry.userId,
|
||||
fileName: entry.fileName,
|
||||
fileSize: entry.fileSize,
|
||||
lastModified: entry.lastModified,
|
||||
mimeType: entry.mimeType,
|
||||
caption: entry.caption ?? '',
|
||||
hashtags: entry.hashtags ?? '',
|
||||
status: entry.status === 'uploading' ? 'pending' : entry.status,
|
||||
progress: entry.status === 'done' ? 100 : 0,
|
||||
error: entry.error
|
||||
};
|
||||
}
|
||||
|
||||
export async function loadQueue(): Promise<void> {
|
||||
const database = await getDb();
|
||||
const myUserId = getUserId();
|
||||
@@ -220,18 +271,7 @@ export async function loadQueue(): Promise<void> {
|
||||
// explicit logout via `clearQueue`).
|
||||
const items: QueueItem[] = all
|
||||
.filter((entry) => entry.userId && entry.userId === myUserId)
|
||||
.map((entry) => ({
|
||||
id: entry.id,
|
||||
userId: entry.userId,
|
||||
fileName: entry.fileName,
|
||||
fileSize: entry.fileSize,
|
||||
mimeType: entry.mimeType,
|
||||
caption: entry.caption ?? '',
|
||||
hashtags: entry.hashtags ?? '',
|
||||
status: entry.status === 'uploading' ? 'pending' : entry.status,
|
||||
progress: entry.status === 'done' ? 100 : 0,
|
||||
error: entry.error
|
||||
}));
|
||||
.map(entryToQueueItem);
|
||||
queueItems.set(items);
|
||||
// Staged-but-unsent items from a prior session (queued offline, tab closed before
|
||||
// reconnect) must resume now — otherwise the "queue flushes when you're back online"
|
||||
@@ -501,10 +541,7 @@ async function uploadItem(id: string): Promise<void> {
|
||||
// = banned) as reversible: an unparseable 403 body (proxy/WAF/captive
|
||||
// portal) must NOT purge the blob — losing a photo is the worst outcome, and
|
||||
// 403 is the reversible-lock status here.
|
||||
if (
|
||||
body?.error === 'uploads_locked' ||
|
||||
(xhr.status === 403 && body?.error !== 'forbidden')
|
||||
) {
|
||||
if (isReversibleLock(xhr.status, body?.error)) {
|
||||
reject(new LockedError(body?.message || 'Event ist geschlossen.'));
|
||||
break;
|
||||
}
|
||||
|
||||
@@ -240,6 +240,7 @@
|
||||
showBottomNav.set(true);
|
||||
clearTimer();
|
||||
if (overlayHideTimer) clearTimeout(overlayHideTimer);
|
||||
if (processedDebounce) clearTimeout(processedDebounce);
|
||||
void releaseWakeLock();
|
||||
for (const unsub of unsubs) unsub();
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user