fix(upload): a retry after release returns the stored photo instead of refusing it
The idempotency key was only readable as a multipart FIELD, and a field cannot be read until the body is being parsed — which happens after the lock/release pre-flight. So the replay was unreachable in exactly the case it exists for: the photo commits → the response is lost on the way back (the flaky-wifi failure the key was added for) → the host releases the gallery at the end of the night → the phone's retry answers `gallery_released`. The guest is told a photo that is sitting in the gallery was never sent. And the remedy the client offers is destructive: `open_event` clears `export_released_at` AND bumps `export_epoch`, retiring the whole keepsake generation and forcing a multi-GB rebuild on a 2-vCPU box at midnight — to re-send a photo that was never missing. Several guests on one flaky evening make this likely to happen at least once. The key is now also sent as `X-Client-Upload-Id`, which arrives with the request line, so the answer is knowable before anything is decided about locks. The multipart field stays for the concurrent case and as a fallback. Placed ahead of the hourly rate limiter too, which was the same mistake one layer up: a 40-photo burst with two retries apiece exhausted the guest's hour on uploads that had all committed the first time. The body is still drained rather than abandoned — replying before reading it makes the proxy see a broken pipe and turn a clean 200 into a 502. The spec carries its own control: a DIFFERENT photo is asserted to still be refused with `gallery_released` after the release, so the replay cannot be green merely because the gate was open.
This commit is contained in:
@@ -168,8 +168,51 @@ const ALLOWED_MEDIA: &[(&str, &str)] = &[
|
|||||||
pub async fn upload(
|
pub async fn upload(
|
||||||
State(state): State<AppState>,
|
State(state): State<AppState>,
|
||||||
auth: AuthUser,
|
auth: AuthUser,
|
||||||
|
headers: axum::http::HeaderMap,
|
||||||
mut multipart: Multipart,
|
mut multipart: Multipart,
|
||||||
) -> Result<(StatusCode, Json<UploadDto>), AppError> {
|
) -> Result<(StatusCode, Json<UploadDto>), AppError> {
|
||||||
|
// REPLAY FIRST — before the rate limit, before the ban check, before the lock/release gate.
|
||||||
|
//
|
||||||
|
// The idempotency key also arrives as a multipart FIELD, and there is a replay for it further
|
||||||
|
// down; but a field cannot be read until the body is being parsed, which is after every gate
|
||||||
|
// below. So the field's replay was unreachable in exactly the situation it matters most:
|
||||||
|
//
|
||||||
|
// the photo committed, the response was lost on the way back (the flaky-wifi failure this
|
||||||
|
// whole mechanism exists for), the host released the gallery at the end of the night, and
|
||||||
|
// the phone's retry then answered `gallery_released` — telling the guest a photo that is
|
||||||
|
// ALREADY IN THE GALLERY had not been sent.
|
||||||
|
//
|
||||||
|
// And the remedy that error suggests is destructive: `open_event` clears `export_released_at`
|
||||||
|
// and BUMPS `export_epoch`, retiring the whole keepsake generation and forcing a multi-GB
|
||||||
|
// rebuild on a 2-vCPU box at midnight — to re-send a photo that was never missing.
|
||||||
|
//
|
||||||
|
// A header arrives with the request line, so the answer is knowable before anything is
|
||||||
|
// decided. Charging the hourly rate limit for a retry of an already-stored photo was the same
|
||||||
|
// mistake one layer up: a 40-photo burst with two retries each exhausted the hour for uploads
|
||||||
|
// that committed the first time.
|
||||||
|
//
|
||||||
|
// The body is still DRAINED rather than abandoned — see `drain_multipart`: replying before
|
||||||
|
// reading the body makes the proxy see a broken pipe and turn a clean 200 into a 502.
|
||||||
|
if let Some(cid) = headers
|
||||||
|
.get("x-client-upload-id")
|
||||||
|
.and_then(|v| v.to_str().ok())
|
||||||
|
.and_then(|v| Uuid::parse_str(v.trim()).ok())
|
||||||
|
&& let Some(existing) =
|
||||||
|
Upload::find_by_client_upload_id(&state.pool, auth.user_id, cid).await?
|
||||||
|
{
|
||||||
|
drain_multipart(multipart).await;
|
||||||
|
let uploader_name = User::find_by_id(&state.pool, auth.user_id)
|
||||||
|
.await?
|
||||||
|
.map(|u| u.display_name)
|
||||||
|
.unwrap_or_default();
|
||||||
|
tracing::info!(
|
||||||
|
client_upload_id = %cid, upload_id = %existing.id,
|
||||||
|
"upload retry replayed from the header key, before the lock/release gate"
|
||||||
|
);
|
||||||
|
let dto = replay_upload_dto(&state, &existing, &uploader_name).await;
|
||||||
|
return Ok((StatusCode::OK, Json(dto)));
|
||||||
|
}
|
||||||
|
|
||||||
// Rate limit: N uploads per hour per user. Gated by master + per-endpoint toggles.
|
// Rate limit: N uploads per hour per user. Gated by master + per-endpoint toggles.
|
||||||
let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await;
|
let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await;
|
||||||
let upload_rate_on = config::get_bool(&state.config_cache, "upload_rate_enabled", true).await;
|
let upload_rate_on = config::get_bool(&state.config_cache, "upload_rate_enabled", true).await;
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ import { BASE } from './env';
|
|||||||
*
|
*
|
||||||
* Field shape matches [backend/src/handlers/upload.rs]:
|
* Field shape matches [backend/src/handlers/upload.rs]:
|
||||||
* - file (binary; carries filename + content_type in the part headers)
|
* - file (binary; carries filename + content_type in the part headers)
|
||||||
|
* - client_upload_id (uuid, optional; also sent as the X-Client-Upload-Id header)
|
||||||
* - caption (text, optional)
|
* - caption (text, optional)
|
||||||
* - hashtags (CSV text, optional)
|
* - hashtags (CSV text, optional)
|
||||||
*/
|
*/
|
||||||
@@ -17,6 +18,12 @@ export type UploadOptions = {
|
|||||||
contentType?: string;
|
contentType?: string;
|
||||||
caption?: string;
|
caption?: string;
|
||||||
hashtags?: string;
|
hashtags?: string;
|
||||||
|
/**
|
||||||
|
* Idempotency key. Sent BOTH as the `X-Client-Upload-Id` header and as the multipart field,
|
||||||
|
* exactly as the real client does — the header is what lets the server replay a stored upload
|
||||||
|
* before it evaluates the lock/release gate, and the field covers the concurrent case.
|
||||||
|
*/
|
||||||
|
clientUploadId?: string;
|
||||||
};
|
};
|
||||||
|
|
||||||
export async function uploadRaw(
|
export async function uploadRaw(
|
||||||
@@ -29,9 +36,12 @@ export async function uploadRaw(
|
|||||||
form.append('file', blob as any, opts.filename ?? 'upload.bin');
|
form.append('file', blob as any, opts.filename ?? 'upload.bin');
|
||||||
if (opts.caption !== undefined) form.append('caption', opts.caption);
|
if (opts.caption !== undefined) form.append('caption', opts.caption);
|
||||||
if (opts.hashtags !== undefined) form.append('hashtags', opts.hashtags);
|
if (opts.hashtags !== undefined) form.append('hashtags', opts.hashtags);
|
||||||
|
if (opts.clientUploadId !== undefined) form.append('client_upload_id', opts.clientUploadId);
|
||||||
|
const headers: Record<string, string> = { Authorization: `Bearer ${token}` };
|
||||||
|
if (opts.clientUploadId !== undefined) headers['X-Client-Upload-Id'] = opts.clientUploadId;
|
||||||
return fetch(`${BASE}/api/v1/upload`, {
|
return fetch(`${BASE}/api/v1/upload`, {
|
||||||
method: 'POST',
|
method: 'POST',
|
||||||
headers: { Authorization: `Bearer ${token}` },
|
headers,
|
||||||
body: form as any,
|
body: form as any,
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
85
e2e/specs/02-upload/retry-after-release.spec.ts
Normal file
85
e2e/specs/02-upload/retry-after-release.spec.ts
Normal file
@@ -0,0 +1,85 @@
|
|||||||
|
/**
|
||||||
|
* A retry of a photo that was ALREADY STORED must return that photo — even after the gallery has
|
||||||
|
* been released.
|
||||||
|
*
|
||||||
|
* The idempotency key arrives as a multipart field as well, and there is a replay for it in the
|
||||||
|
* handler; but a field cannot be read until the body is being parsed, which happens after the
|
||||||
|
* lock/release pre-flight. So the replay was unreachable in precisely the case that matters:
|
||||||
|
*
|
||||||
|
* the photo commits → the response is lost on the way back (the flaky-wifi failure the key
|
||||||
|
* exists for) → the host releases the gallery at the end of the night → the phone retries →
|
||||||
|
* `gallery_released`.
|
||||||
|
*
|
||||||
|
* The guest is then told a photo that is sitting in the gallery was never sent, and the remedy the
|
||||||
|
* client offers — ask the hosts to reopen — bumps `export_epoch`, retiring the whole keepsake and
|
||||||
|
* forcing a rebuild, to re-send something that was never missing.
|
||||||
|
*
|
||||||
|
* The key is now also sent as `X-Client-Upload-Id`, which arrives with the request line.
|
||||||
|
*/
|
||||||
|
import { test, expect } from '../../fixtures/test';
|
||||||
|
import { uploadRaw } from '../../helpers/upload-client';
|
||||||
|
import { readFileSync } from 'node:fs';
|
||||||
|
import { join } from 'node:path';
|
||||||
|
import { BASE } from '../../helpers/env';
|
||||||
|
|
||||||
|
const SLUG = 'e2e-test-event';
|
||||||
|
|
||||||
|
function sample(): Buffer {
|
||||||
|
return readFileSync(join(process.cwd(), 'fixtures', 'media', 'sample.jpg'));
|
||||||
|
}
|
||||||
|
|
||||||
|
test.describe('Upload — a retry after release replays instead of refusing', () => {
|
||||||
|
test('the stored photo comes back, and the guest is not told to reopen the gallery', async ({
|
||||||
|
guest,
|
||||||
|
db,
|
||||||
|
}) => {
|
||||||
|
const g = await guest('WiederholerWilli');
|
||||||
|
const key = crypto.randomUUID();
|
||||||
|
|
||||||
|
// 1. The upload commits. In the real failure the guest never sees this response.
|
||||||
|
const first = await uploadRaw(g.jwt, sample(), {
|
||||||
|
filename: 'a.jpg',
|
||||||
|
contentType: 'image/jpeg',
|
||||||
|
clientUploadId: key,
|
||||||
|
});
|
||||||
|
expect(first.status).toBe(201);
|
||||||
|
const original = await first.json();
|
||||||
|
|
||||||
|
// 2. The host releases the gallery — the end-of-event action every queue runs into.
|
||||||
|
await db.setExportReleased(SLUG, true);
|
||||||
|
|
||||||
|
// A DIFFERENT photo must still be refused: this is the control that proves the release is
|
||||||
|
// actually in effect, so the replay below is not just "the gate was open all along".
|
||||||
|
const stranger = await uploadRaw(g.jwt, sample(), {
|
||||||
|
filename: 'b.jpg',
|
||||||
|
contentType: 'image/jpeg',
|
||||||
|
clientUploadId: crypto.randomUUID(),
|
||||||
|
});
|
||||||
|
expect(stranger.status, 'a genuinely new upload must still be refused after release').toBe(403);
|
||||||
|
expect((await stranger.json()).error).toBe('gallery_released');
|
||||||
|
|
||||||
|
// 3. The phone retries the FIRST photo. It is already in the gallery, so the honest answer is
|
||||||
|
// the stored row — not "the gallery is closed".
|
||||||
|
const retry = await uploadRaw(g.jwt, sample(), {
|
||||||
|
filename: 'a.jpg',
|
||||||
|
contentType: 'image/jpeg',
|
||||||
|
clientUploadId: key,
|
||||||
|
});
|
||||||
|
expect(
|
||||||
|
retry.status,
|
||||||
|
'a retry of an already-stored photo must be replayed, not refused with gallery_released'
|
||||||
|
).toBe(200);
|
||||||
|
const replayed = await retry.json();
|
||||||
|
expect(replayed.id, 'the replay must return the ORIGINAL upload, not a new one').toBe(
|
||||||
|
original.id
|
||||||
|
);
|
||||||
|
|
||||||
|
// 4. And no second row was created — the whole point of the key.
|
||||||
|
const feed = await fetch(`${BASE}/api/v1/feed?limit=100`, {
|
||||||
|
headers: { Authorization: `Bearer ${g.jwt}` },
|
||||||
|
});
|
||||||
|
const items: any[] = (await feed.json()).uploads ?? [];
|
||||||
|
const mine = items.filter((u) => u.id === original.id);
|
||||||
|
expect(mine.length, 'the photo must appear exactly once').toBe(1);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -1127,6 +1127,17 @@ async function uploadItem(id: string): Promise<void> {
|
|||||||
const xhr = new XMLHttpRequest();
|
const xhr = new XMLHttpRequest();
|
||||||
xhr.open('POST', '/api/v1/upload');
|
xhr.open('POST', '/api/v1/upload');
|
||||||
xhr.setRequestHeader('Authorization', `Bearer ${token}`);
|
xhr.setRequestHeader('Authorization', `Bearer ${token}`);
|
||||||
|
// The SAME idempotency key as the multipart field above, in a header.
|
||||||
|
//
|
||||||
|
// The field alone cannot be read until the body is being parsed, which is after the
|
||||||
|
// release/lock pre-flight — so a retry of a photo that was already stored got
|
||||||
|
// `gallery_released` instead of its original row, and the guest was told a photo that
|
||||||
|
// IS in the gallery had not been sent. The only remedy on offer (ask the hosts to
|
||||||
|
// reopen) bumps the export epoch and destroys the released keepsake.
|
||||||
|
//
|
||||||
|
// A header arrives with the request line, so the server can replay before it decides
|
||||||
|
// anything about locks. The field stays for the concurrent case and as the fallback.
|
||||||
|
xhr.setRequestHeader('X-Client-Upload-Id', entry.id);
|
||||||
// Wall-clock backstop only — generous enough that a slow-but-alive LTE upload is
|
// Wall-clock backstop only — generous enough that a slow-but-alive LTE upload is
|
||||||
// never killed by it. See MIN/MAX_UPLOAD_TIMEOUT_MS.
|
// never killed by it. See MIN/MAX_UPLOAD_TIMEOUT_MS.
|
||||||
xhr.timeout = Math.min(
|
xhr.timeout = Math.min(
|
||||||
|
|||||||
Reference in New Issue
Block a user